RFR: 8290840: Refactor the "os" class [v2]

David Holmes dholmes at openjdk.org
Tue Jul 26 00:03:59 UTC 2022


On Mon, 25 Jul 2022 22:54:15 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 incrementally with two additional commits since the last revision:
> 
>  - moved os::{print_active_locale, print_user_info} to os_posix.cpp
>  - Fixed os.hpp comments per @dholmes-ora review

Updates look good thanks. Overall this seems good to me. I think there may be more refinement possible with regards to the os::XXX classes, but that can be future cleanup if you want to proceed with this version for now.

Thanks.

Mis-clicked. Approved.

src/hotspot/os/posix/os_posix.cpp line 573:

> 571:   ::umask(umsk);
> 572:   st->print("umask: %04o (", (unsigned) umsk);
> 573:   os::Posix::print_umask(st, umsk);

There's really no reason `print_umask` has to be part of the os::Posix class, it could just be a static file function in os_posix.cpp. I suspect the same may be true for other things in os::Posix - I would think we only need things in os::Posix that are explicitly called from os::Linux or os::Bsd etc.

-------------

PR: https://git.openjdk.org/jdk/pull/9600Marked as reviewed by dholmes (Reviewer).


More information about the hotspot-dev mailing list