RFR: 8350864: C2: verify structural invariants of the Ideal graph [v5]

[Marc Chevalier]

On Mon, 8 Sep 2025 15:59:58 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   One more ResourceMark
>
> src/hotspot/share/opto/graphInvariants.cpp line 222:
> 
>> 220:     }
>> 221:     return result;
>> 222:   }
> 
> Would this not read better?
> Suggestion:
> 
>     bool success = _pattern->check(center->in(_which_input), state);
>     if (!success) {
>       state.trace_failure_path(center, _which_input);
>     }
>     return success;
>   }

I don't think it's terrible, but I don't think it's much better. If I know the code and that we want to add the new points in the path, and I'll read it as such either way. Or I don't know the code, but I know the types of `steps` and `path`, and I know what `push` does, while a custom type with custom methods has an higher learning cost. So to me, it's pretty equivalent.

> src/hotspot/share/opto/graphInvariants.cpp line 239:
> 
>> 237:     return true;
>> 238:   }
>> 239:   bool (Node::*_type_check)() const;
> 
> Suggestion:
> 
> private:
>   bool (Node::*_type_check)() const;
> 
> I would also suggest that you use a `typedef` here.
> Something like:
> `typedef bool (Node::*TypeCheckMethod)() const;`
> Then you can write
> Suggestion:
> 
> public:
>   const TypeCheckMethod _type_check;

Again, is it really better? If I know the code, I know what it needs, however I express it. If I don't know the code, looking at the signature won't be enough, I'll need to look up one level deeper the definition. Not sure it's a win.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2332328768
PR Review Comment: https://git.openjdk.org/jdk/pull/26362#discussion_r2332333010