On Mon, 13 May 2024 04:55:24 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> MEMFLAGS, as well as its enum constants, should live in its own include. >> >> The constants are used throughout the code base, often without needing the >> allocation APIs exposed through allocation.hpp. >> >> The MEMFLAGS enum def is often needed within NMT itself, again often without >> needing allocation.hpp. >> >> --- >> >> This patch moves the enum to its new file. >> >> It fixes those `allocation.hpp` includes that where only needed to get >> MEMFLAGS. It does not fix other includes. >> >> For backward compatibility, until we straightened out the dependencies >> (e.g., fixing all places where we rely on indirect includes), I added >> memflags.hpp to allocation.hpp. >> >> I tested (built) on: >> - MacOS aarch64, no precompiled headers, fastdebug >> - Linux x64, no precompiled headers, fastdebug, release, fastdebug >> crossbuild to aarch64, fastdebug minimal > > Thomas Stuefe has updated the pull request incrementally with one additional > commit since the last revision: > > Update mallocLimit.hpp Changes requested by stefank (Reviewer). src/hotspot/share/nmt/mallocTracker.hpp line 29: > 27: #define SHARE_NMT_MALLOCTRACKER_HPP > 28: > 29: #include "nmt/memflags.hpp" Should go after mallocHeader.hpp src/hotspot/share/nmt/memflags.cpp line 27: > 25: #include "precompiled.hpp" > 26: > 27: #include "nmt/memflags.hpp" There should be no blankline between precompiled.hpp and the rest of the includes. src/hotspot/share/nmt/memflags.cpp line 31: > 29: > 30: // Extra insurance that MEMFLAGS truly has the same size as uint8_t. > 31: STATIC_ASSERT(sizeof(MEMFLAGS) == sizeof(uint8_t)); I think you can remove this entire .cpp file. There's no need to check the size of an enum with a specified base type. src/hotspot/share/nmt/memflags.hpp line 30: > 28: #include "utilities/globalDefinitions.hpp" > 29: > 30: #define MEMORY_TYPES_DO(f) > \ Open-ended comment/question: We call it MEMORY_TYPE and mt, but then we call the type MEMFLAGS (with a completely non-standard UPPERCASE style). Maybe it is time to rename MEMFLAGS? src/hotspot/share/services/mallocLimit.cpp line 28: > 26: #include "precompiled.hpp" > 27: > 28: #include "nmt/memflags.hpp" While poking around in the includes, could you remove the blankline on 27. This style inconsistency has slowly crept into the code base. ------------- PR Review: https://git.openjdk.org/jdk/pull/19172#pullrequestreview-2052269037 PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598224275 PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598225428 PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598226640 PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598229830 PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598231329
