RFR: 8337563: NMT: rename MEMFLAGS to MemFlag

Kim Barrett kbarrett at openjdk.org
Fri Aug 30 10:06:20 UTC 2024


On Thu, 29 Aug 2024 16:17:09 GMT, Stefan Karlsson <stefank at 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


More information about the serviceability-dev mailing list