RFR: 8290840: Refactor the "os" class [v6]
Stefan Karlsson
stefank at openjdk.org
Tue Aug 2 16:52:20 UTC 2022
On Tue, 2 Aug 2022 16:06:10 GMT, Ioi Lam <iklam at openjdk.org> wrote:
>> Please see [JDK-8290840](https://bugs.openjdk.org/browse/JDK-8290840) for the detailed proposal.
>>
>> The `os` class, declared in os.hpp, forms the major part of the HotSpot porting interface. Its structure has gradually deteriorated over the years as new ports are created and new APIs are added.
>>
>> This RFE tries to address the following:
>>
>> - Clearly specify where a porting API should be declared and defined among the various `os*.cpp` and `os*.hpp` files.
>> - Avoid the inappropriate inclusion of OS-specific APIs (such as the `os::Linux class`) by platform-independent source files.
>
> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 13 additional commits since the last revision:
>
> - clarified comments about inline functions declared in os.hpp
> - Merge branch 'master' into 8290840-refactor-the-os-class
> - made workaround_expand_exec_shield_cs_limit() static; added PPC comments for os::resolve_function_descriptor()
> - fixed whitespace
> - clarify comment
> - PLATFORM_PRINT_NATIVE_STACK -> HAVE_PLATFORM_PRINT_NATIVE_STACK
> - simplified rarely-defined function os::resolve_function_descriptor()
> - Made workaround_expand_exec_shield_cs_limit() a static function in os_linux.cpp
> - fixed typo
> - moved os::{print_active_locale, print_user_info} to os_posix.cpp
> - ... and 3 more: https://git.openjdk.org/jdk/compare/eae2b95f...b6e60a54
I love seeing the os files being cleaned up and restructured.
I started to look at the patch and found a significant number of nits that I think would be good to fix.
src/hotspot/os_cpu/linux_zero/os_linux_zero.cpp line 37:
> 35: #include "nativeInst_zero.hpp"
> 36: #include "os_posix.hpp"
> 37: #include "os_linux.hpp"
Sort
src/hotspot/share/runtime/os.inline.hpp line 31:
> 29:
> 30: #include OS_HEADER(os)
> 31: #include OS_HEADER_INLINE(os)
We usually only explicitly include .inline.hpp and don't and let that file bring in the corresponding .hpp file.
test/hotspot/gtest/runtime/test_os.cpp line 37:
> 35: #include "runtime/frame.inline.hpp"
> 36: #include "runtime/threads.hpp"
> 37:
Remove blank line
-------------
Changes requested by stefank (Reviewer).
PR: https://git.openjdk.org/jdk/pull/9600
More information about the hotspot-dev
mailing list