RFR: 8340103: Add internal set_flag function to VMATree [v11]

[Thomas Stuefe]

On Tue, 5 Nov 2024 08:09:58 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> src/hotspot/share/nmt/nmtTreap.hpp line 334:
>> 
>>> 332:     TreapNode* start = closest_leq(addr);
>>> 333:     TreapNode* end = closest_gt(addr);
>>> 334:     return Range{start, end};
>> 
>> I am fine with curly bracket initializer, but does it have to be so dense? What does the styleguide say? I never wrote plain arrays like this, always with spaces interleaved (`int[] i = { 1, 2, 3 };`)
>
> I'd just do curly brace initialization the same way as paren init. I don't think our style guide says anything, but the typical style that I see in C++ code is this dense variant.
> 
> Edit: See example code in https://en.cppreference.com/w/cpp/language/aggregate_initialization and search for "uniform init" in https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines

But it is not uniform if we end up with three different initialization styles: the old way of `object(a, b, c);` the new way of `object{a, b, c};` and plain array/struct init, which  overwhelmingly seems to use spaces around curlies: `= { a, b, c };` .

>> src/hotspot/share/nmt/vmatree.hpp line 151:
>> 
>>> 149:   IntervalState& out_state(TreapNode* node) {
>>> 150:     return node->val().out;
>>> 151:   }
>> 
>> Why are these instance methods? Also, TreapNode* should be const. But maybe we can do things even better.
>> 
>> In cases like these I sometimes just use an ephemeral utility class. E.g.
>> 
>> 
>> class VMATreeNode {
>> const TreapNode* const _n;
>> public:
>> VMATreeNode(const TreapNode* node) .. assign _n
>> MemTag mem_tag_in() const { return _n->val().in.mem_tag();  } 
>> MemTag mem_tag_out() const { return _n->val().out.mem_tag();  } 
>> ... all accessors in accessible form to save writing out the many dereferences ..
>> };
>> 
>> 
>> and then just use it as I go like this:
>> 
>> 
>> TreapNode* node = ...
>> if (VMATreeNode(node).mem_tag_in() == blabla )....
>> 
>> Bonus for a short name for VMATreeNode, because the code that really does anything then becomes a lot more readable.
>
> They should be static and const, yes.
> 
> Re: ephemeral utility class. I do prefer using functions as opposed to methods, but it'd be nice to wrap them in a namespace or class that can be shortened and used outside of `VMATree`.

Okay

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20994#discussion_r1828980988
PR Review Comment: https://git.openjdk.org/jdk/pull/20994#discussion_r1828981787