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