On Mon, 13 May 2024 14:31:22 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> 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. > > I rather have this explicit check. If MEMFLAGS>1byte, things break, and I > would like to make that explicit. > > That said, I can move this static assert to the header. I just wanted to > avoid including debug.hpp. My original intent was for this cpp file to be the > place in the future for any MEMFLAGS related utility functions, e.g. > to-and-from-string conversations. Could you instead put the static_assert near the code that will break? Right now it looks obscure and weird to have this check when it is obviously correct as long as no one changes the definition. Would it be enough to write a comment in the header that this needs to be 1 byte? >> 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? > > I don't feel like starting that particular bike shedding discussion :) But > sure, sometime in the future we should do this. Here, I want it to be a > simple renaming change. Right. That's why I prefixed this with "Open-ended comment/question", trying to make it super clear that it wasn't intended as a request for this PR, but rather a way to at least plant the seed of an idea that we might want to fix this eyesore. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598603277 PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598608110
