Two thoughts on equals

[lombokissues]

Migrated from Google Code (issue 366)

[lombokissues]

👤 k.tinnefeld   🕗 Apr 21, 2012 at 09:48 UTC

In a lombok-generated equal, we read

1. double getter call

if (this.getName() == null ? other.getName() != null : !this.getName().equals((java.lang.Object)other.getName())) return false;

This gives me a bad feeling about calling a method (be it "just" a getter) twice. Even though there are workarounds (lazy getters for finals and the option to read fields directly), I would suggest - at least as an option - to generate sth like

final java.lang.Object $lombok$eq$t$name = this.getName();
final java.lang.Object $lombok$eq$o$name = other.getName();
if ($lombok$eq$t$name == null ? $lombok$eq$o$name != null : !$lombok$eq$t$name.equals($lombok$eq$o$name)) return false;

2. (Really minor) order optimization

final SimpleModel other = (SimpleModel)o;
if (!other.canEqual((java.lang.Object)this)) return false;

Shouldn't this be the other way round? No need to cast if canEqual() fails.

[lombokissues]

👤 k.tinnefeld   🕗 Apr 21, 2012 at 20:08 UTC

What version of the product are you using? On what operating system?

0.11.0 on Windows 7 64 Bit

[lombokissues]

👤 r.spilker   🕗 Apr 23, 2012 at 19:04 UTC

2nd point: We can't call canEqual without first casting; canEqual is not a method that java.lang.Object has. The way canEqual works, calling this.canEqual(o) doesn't do anything useful, the whole point of the call is to ask the other object.

As to point ﹟1: it is a good idea to only call the getter on the this object only once and store the result in a local variable. There is no need to also call the call to the other object since it will only be called once anyway, albeit on two different locations.

For the naming of the local variable: we can just use one simple name like "object" and use it for all the fields containing an object reference.

[lombokissues]

👤 r.spilker   🕗 Apr 23, 2012 at 19:06 UTC

We should also call the getter in hashCode only once as well.

[lombokissues]

👤 k.tinnefeld   🕗 Apr 24, 2012 at 20:13 UTC

Sure, forget about 2nd.

We should also call the getter once in LONG.equals. I chose "field" over "object" as field name, though.

The attachment does the javac changes, didn't look into the eclipse code too deep yet.

[lombokissues]

👤 k.tinnefeld   🕗 Apr 24, 2012 at 20:13 UTC

🔗 lombok-#366-javac.patch

[lombokissues]

👤 r.spilker   🕗 Apr 25, 2012 at 08:57 UTC

Thanks for dedicating the time to create a patch. In the mean time we've been working on this issue as well. To prevent double work, next time please contact us through the issue if you plan to create a patch.

In our implementation we've modified the following:

• Always use a local variable if we read a value twice, even for fields (Android currently cannot optimize field access)
• Create the local variable just before its use instead of on top of the hashCode method (smaller stack-size)
• Better naming for the local variables ($fieldName in hashCode, this$fieldName and other$fieldName in equals)
• No longer need to cast to java.lang.Object in equals (since the local variable is alreadt of type Object)
• Updated all test cases to reflect the changes above.

If you plan to create a patch in the future, which is highly appreciated, please clone the github repository, and add your name to the AUTHORS file as well. That way we can also ensure we don't run into legal problems in the future. And we can give feedback before integrating it into Lombok.

[lombokissues]

👤 r.spilker   🕗 Apr 25, 2012 at 08:58 UTC

We still need to work on the Eclipse implementation, so the source is not yet pushed to github.

[lombokissues]

👤 r.spilker   🕗 Apr 29, 2012 at 21:59 UTC

Fixed in 25def86

[lombokissues]

👤 r.spilker   🕗 Apr 29, 2012 at 21:59 UTC

[lombokissues]

End of migration