Skip to content
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

Suggest Double.compare in ObjectEqualsForPrimitives #4394

Closed
wants to merge 1 commit into from

Conversation

findepi
Copy link
Contributor

@findepi findepi commented May 14, 2024

Objects.equal(a_double, another_double) and a_double == another_double have different semantics (different result when both compared values are NaN). Suggest replacement using Double.compare to avoid changing code semantics. This is especially relevant when implementing equals methods.

Fixes #4392

@findepi
Copy link
Contributor Author

findepi commented May 14, 2024

@cushon can you please approve the build to run?

cc @msridhar you might be interesting in this in context of automated code fixes.

@cushon
Copy link
Collaborator

cushon commented May 15, 2024

We have some internal guidance to use Double.toLongBits when implementing exact equality on doubles if the values could be NaN, instead of Double.compare. What do you think of that alternative?

I'm also wondering if we should exclude double and float from this change, since it's just a micro-optimization, and there's a readability tradeoff with using Double.compare or Double.toLongBits

@findepi
Copy link
Contributor Author

findepi commented May 15, 2024

@cushon thank you for your review!

We have some internal guidance to use Double.toLongBits when implementing exact equality on doubles if the values could be NaN, instead of Double.compare. What do you think of that alternative?

Double.doubleToRawLongBits is obviously not a suitable replacement, since it compares two different NaN representations as numerically different, while Double.equals(Object) compares them as equal (and so does Double.compare(double, double).

Double.doubleToLongBits looks as a good alternative & may perform better, changed the PR, thanks!

Would you mind triggering the build again?

I'm also wondering if we should exclude double and float from this change, since it's just a micro-optimization, and there's a readability tradeoff with using Double.compare or Double.toLongBits

i am not sure i understand. Can you please rephrase?

@cushon
Copy link
Collaborator

cushon commented May 15, 2024

I'm also wondering if we should exclude double and float from this change, since it's just a micro-optimization, and there's a readability tradeoff with using Double.compare or Double.toLongBits

i am not sure i understand. Can you please rephrase?

I was wondering about having ObjectEqualsForPrimitives not emit findings for double and float. There's a tradeoff between Objects.equal(a_double, another_double) and Double. toLongBits(a_double) == Double. toLongBits(another_double), the latter avoids some boxing and is theoretically faster, but the first is potentially more readable.

@findepi
Copy link
Contributor Author

findepi commented May 16, 2024

Sure, I can do that.

`Objects.equal(a_double, another_double)` and `a_double ==
another_double` have different semantics (different result when both
compared values are NaN). Suggest replacement using `Double.compare` to
avoid changing code semantics. This is especially relevant when
implementing `equals` methods.
@findepi
Copy link
Contributor Author

findepi commented May 16, 2024

@cushon Would you mind triggering the build again?

@findepi
Copy link
Contributor Author

findepi commented May 16, 2024

@cushon thanks for approval.
I see the JDK EA on ubuntu-latest build is failing. I don't know how to fix it.

@cushon
Copy link
Collaborator

cushon commented May 16, 2024

JDK EA is also failing at head, there are some breaking changes we'll need to prepare for before JDK 23 is released, but it doesn't need to be a blocker for this

copybara-service bot pushed a commit that referenced this pull request May 16, 2024
`Objects.equal(a_double, another_double)` and `a_double == another_double` have different semantics (different result when both compared values are NaN). This is especially relevant when implementing `equals` methods.

Fixes #4392

Fixes #4394

FUTURE_COPYBARA_INTEGRATE_REVIEW=#4394 from findepi:findepi/double-compare 603b9c6
PiperOrigin-RevId: 634390506
@findepi findepi deleted the findepi/double-compare branch May 16, 2024 15:36
@findepi
Copy link
Contributor Author

findepi commented May 16, 2024

thanks for the merge!

@findepi
Copy link
Contributor Author

findepi commented May 16, 2024

oh, and thanks for fixing the commit message.

@cushon
Copy link
Collaborator

cushon commented May 16, 2024

thanks for the merge!

sure thing!

oh, and thanks for fixing the commit message.

@cpovirk gets the credit for that part :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants