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

[Jasmine Karthikeyan]

On Mon, 15 Apr 2024 10:22:00 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> 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.

This is a good point, I hadn't thought about backports! In the top comment I was also thinking about naming it `Type::equals`, but I worried that it'd be too close to `Type::eq`. But with this detail I think it should be renamed as well, because of the change of semantics. That way at least it'll cause a compile error instead of silently breaking at runtime, which would be bad.

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

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