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

Use Comparable instead of dedicated implementations #25478

Closed
wants to merge 4 commits into from

Conversation

wind57
Copy link
Contributor

@wind57 wind57 commented Jul 27, 2020

No description provided.

@pivotal-issuemaster
Copy link

@wind57 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@wind57 Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jul 27, 2020
@@ -63,8 +64,9 @@ public BooleanComparator(boolean trueLow) {


@Override
public int compare(Boolean v1, Boolean v2) {
return (v1 ^ v2) ? ((v1 ^ this.trueLow) ? 1 : -1) : 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some time ago I had an answer about this here : https://stackoverflow.com/questions/55598277/is-there-a-useful-difference-between-p-q-and-p-q-for-booleans/55601399#55601399 and it turns out that the performance of XOR vs plain != is same. This also makes it far easier to read

@@ -76,7 +78,7 @@ public boolean equals(@Nullable Object other) {

@Override
public int hashCode() {
return getClass().hashCode() * (this.trueLow ? -1 : 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since java-8 there is a way to get the hashCode without calling into getClass

@@ -29,34 +29,35 @@
* @author Keith Donald
* @author Chris Beams
* @author Phillip Webb
* @author Eugene Rabii
*/
class BooleanComparatorTests {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests have changed because the documentation of Boolean::compare does not guarantee -1, 0, 1; but it does less than 1, 0, greater than 1

@wind57 wind57 closed this Jul 27, 2020
@wind57
Copy link
Contributor Author

wind57 commented Jul 27, 2020

Will provide a broader PR, against the entire package.

@snicoll
Copy link
Member

snicoll commented Jul 27, 2020

@wind57 please do not do that. If you want to update your PR, you can push more changes to your comparator-cleanup branch and this will update this PR.

@wind57 wind57 reopened this Jul 28, 2020
@@ -29,49 +29,44 @@

/**
* Return a {@link Comparable} adapter.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used Comparator::naturalOrder and Comparator::nullLast/nullFirst instead of NullSafeComparator

@wind57
Copy link
Contributor Author

wind57 commented Jul 28, 2020

@snicoll thank you, I am still new, not sure about the process yet. re-opened.

@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement labels Jul 28, 2020
@snicoll snicoll changed the title BooleanComparator cleanup Use Comparable instead of dedicated implementations Aug 25, 2023
@snicoll snicoll self-assigned this Aug 25, 2023
@snicoll snicoll removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 25, 2023
@snicoll snicoll added this to the 6.1.0-RC1 milestone Aug 25, 2023
snicoll pushed a commit that referenced this pull request Aug 25, 2023
This commit deprecates ComparableComparator and NullSafeComparator in
favor of the available convenient implementation in the JDK.

See gh-25478
@snicoll snicoll closed this in 178cb42 Aug 25, 2023
@snicoll
Copy link
Member

snicoll commented Aug 25, 2023

@wind57 thanks very much for making your first contribution to the Spring Framework. I've polished your change and deprecated those two implementations as we can now replace them by the convenience method in the JDK.

sbrannen pushed a commit that referenced this pull request Dec 11, 2023
Commit 33454a4 introduced a regression in Comparators.nullsLow() and
Comporators.nullsHigh().

This commit updates the code so that nullsLow() sorts null values lower
than non-null values and nullsHigh sorts null values higher than
non-null values.

See gh-25478
Closes gh-31808
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants