Modify

Opened 13 days ago

Closed 13 days ago

Last modified 12 days ago

#19281 closed enhancement (fixed)

[PATCH] Use Objects.hash where it is not used

Reported by: hiddewie Owned by: team
Priority: trivial Milestone: 20.05
Component: Core Version:
Keywords: refactoring Cc:

Description

In many of the JOSM classes, the Objects.hash(...) function has been used to simplify the generation of an int hashCode() method.

This patch uses this in some leftover methods that still had a generated implementation.

Attachments (1)

Objects_hash_simplifications1.patch (23.6 KB) - added by hiddewie 13 days ago.

Download all attachments as: .zip

Change History (12)

Changed 13 days ago by hiddewie

comment:1 Changed 13 days ago by simon04

Keywords: refactoring added
Milestone: 20.05

Parent ticket: #19174

We don't have to keep the >>> 32, this originates from java.lang.Double#hashCode(double) and will be called when using Objects.hash. I'll update your patch and commit it.

Thank you!

comment:2 Changed 13 days ago by simon04

Resolution: fixed
Status: newclosed

In 16488/josm:

fix #19281, see #19174 - Use Objects.hash where it is not used (patch by hiddewie, modified)

comment:4 Changed 13 days ago by GerdP

Simple typo?
return Objects.hash(lhs, lhs);
should be
return Objects.hash(lhs, rhs);

comment:5 Changed 13 days ago by GerdP

In 16491/josm:

see #19281: fix typo

comment:6 Changed 12 days ago by GerdP

In 16493/josm:

see #19281: fix another typo reported by EqualsVerifier (super.hashCode was missing)

comment:7 Changed 12 days ago by hiddewie

Thanks for those fixes. Is there a way to display static analysis and test results before patches get merged? This would prevent errors in the 'master' branch.

comment:8 Changed 12 days ago by GerdP

I only know the ant tasks checkstyle, checkstyle-changed, spotbugs, and pmd and of course clean dist to begin with.

comment:9 Changed 12 days ago by simon04

@GerdP, thank you!

Those testEqualsContract are standard unit tests (test/unit/org/openstreetmap/josm/data/osm/search/SearchCompilerTest.java). Those can be run from the IDE, or from the commandline using the following command:

ant test '-Ddefault-junit-includes=**/SearchCompilerTest.class'

comment:10 Changed 12 days ago by hiddewie

Yes I did not run all tests locally and should have of course.

My question was more about a visual feedback within this ticket about test results from CI, before they are merged to the master branch. I see there is work going on to migrate to a GitLab instance (#16871) which would give such functionality out of the box.

I am willing to help setting up some pipelines up in GitLab if there are concrete tasks.

comment:11 Changed 12 days ago by simon04

The GitLab server is not ready to be used, unfortunately.

What we've got in place at the moment is Travis CI setup for the GitHub mirror – https://travis-ci.org/github/openstreetmap/josm. As you can tell from the number of failing builds, it hasn't gotten any love in the past months. Opening a PR for https://github.com/openstreetmap/josm triggers this CI, however, since it is just a mirror, we cannot merge the PRs via GitHub but have to apply them manually via SVN.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.