On Thu, 29 Aug 2024 16:17:09 GMT, Stefan Karlsson <stef...@openjdk.org> wrote:
>> Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. >> >> `MEMFLAGS` implies that we can use more than one at the same time, but those >> are exclusive values, so `MemType` is much more suitable name. >> >> There is a bunch of other related cleanup that we can do, but I will leave >> for follow up issues such as [NMT: rename NMTUtil::flag to >> NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) > > I much prefer to see MemType, but I'm warming up to NMTCategory. > > - MemType: Succinct - matches part of the code (E.g. the mt in mtGC) > - MemTypeFlag: Too many words for my preference. > - NMTCat: Meuw. :) > - NMTCategory: Parts of the code call these categories, so I'm not entirely > against this. > - NMTGroup: "Group" is a new name for this that currently isn't reflected at > all in the code. > - NMT_MemType: I think we should try get rid of names using this style. > - NMT::MemType: The `::` makes all function declarations noisier for very > little benefit, IMO. > > I continue to mostly agree with @stefank. > > I think this name shouldn't be considered in isolation. There are already a > > bunch of "NMT_" prefixed names. That's the common idiom for things like > > this (often (maybe even usually?) without the "_"). Why are we proposing to > > adopt a new style. (For just this? That would be weird. Or more broadly? > > That certainly needs more discussion.) > > It is precisely because we already are using `NMT_TrackingStackDepth` and > `NMT_TrackingLevel` (and the fact that MemType by itself was too general) > that I suggested we adopt `NMT::` Yes, it would be all static class. > > Why not all lower letter `nmt::`? Again, because we already use `NMT_` > elsewhere. > > My plan was later to switch from `NMT_` to `NMT::` for all of them. We can do > `nmt::` too. An `NMT_` => `NMT::` change requires using a namespace. An allstatic class is closed for extension, and some of these definitions are in different files. The only way to use an allstatic class instead of a namespace is to go the same route as currently used by the "os" class (over my dead body, and probably @tstuefe too). That previously unstated plan also supports my claim that we shouldn't be considering this one type's name in isolation. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2320728486
