RFR: 8329194: Cleanup Type::cmp definition and usage

[Christian Hagedorn]

On Mon, 15 Apr 2024 09:53:11 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Hi all, this patch aims to cleanup `Type::cmp` by changing it from returning a `0` when types are equal and `1` when they are not, to it returning a boolean denoting equality. This makes its usages at various callsites more intuitive. However, as it is passed to the type dictionary as a comparator, a lambda is needed to map the boolean to a comparison value. 
>> 
>> I was also considering changing the name to `Type::equals` as it's not really returning a comparison value anymore, but I felt it would be too similar to `Type::eq`. If this would be preferred though, I can change it.
>> 
>> Tier 1 testing passes on my machine. Reviews and thoughts would be appreciated!
>
> src/hotspot/share/opto/type.hpp line 219:
> 
>> 217:   static const Type *make(enum TYPES);
>> 218:   // Test for equivalence of types
>> 219:   static bool cmp(const Type* t1, const Type* t2);
> 
> I wonder if this will be a bit confusing to people now. Usually, `cmp` methods tend to have an output that allows sorting, i.e. `0` for equals, and either larger or smaller than `0`, depending on which one is larger.
> 
> For a `bool` return, the name `equals` would be more adequate. But this would also require lots of changes in the code-base, and maybe make backports harder.
> 
> Actually, I'd be worried about backports now in general:
> Imagine someone now writes `if (Type::cmp(...))`. And someone else backports this, since the patch seems to cleanly apply. `if (Type::cmp(...))` is also correct C++ before your change.
> 
> But the semantics now have changed: with your patch, the `if` succeeds if the types are equal. Without your patch, the if succeeds if the types are not equal. Yikes. This will lead to some very subtle bugs and annoying debugging in old JDK versions.
> 
> Hence, I would think you have to rename the method to `equals` and change all usages accordingly.
> 
> What do you think?

That's actually a good point about the backports. I was also wondering if `equals` is better due to the `cmp` convention of using ints but thought it's okay. But with backports in mind, it gives a stronger case to actually go with `equals` - so, I agree with your suggestion @eme64.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18533#discussion_r1565546151