New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two thoughts on equals #439

Closed
lombokissues opened this Issue Jul 14, 2015 · 11 comments

Comments

Projects
None yet
2 participants
@lombokissues
Collaborator

lombokissues commented Jul 14, 2015

Migrated from Google Code (issue 366)

@lombokissues

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

👤 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;

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

Collaborator

lombokissues commented Jul 14, 2015

👤 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;

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

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

👤 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

Collaborator

lombokissues commented Jul 14, 2015

👤 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

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

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

Collaborator

lombokissues commented Jul 14, 2015

👤 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

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

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

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

Collaborator

lombokissues commented Jul 14, 2015

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

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

@lombokissues

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

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

Collaborator

lombokissues commented Jul 14, 2015

👤 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

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

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

🔗 lombok-#366-javac.patch View file

Collaborator

lombokissues commented Jul 14, 2015

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

🔗 lombok-#366-javac.patch View file

@lombokissues

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

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

Collaborator

lombokissues commented Jul 14, 2015

👤 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

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

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

Collaborator

lombokissues commented Jul 14, 2015

👤 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

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

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

Fixed in 25def86

Collaborator

lombokissues commented Jul 14, 2015

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

Fixed in 25def86

@lombokissues lombokissues added this to the 0.11.2 milestone Jul 14, 2015

@lombokissues

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

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

Collaborator

lombokissues commented Jul 14, 2015

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

@lombokissues

This comment has been minimized.

Show comment
Hide comment
@lombokissues

lombokissues Jul 14, 2015

Collaborator

End of migration

Collaborator

lombokissues commented Jul 14, 2015

End of migration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment