Community
Participate
Working Groups
A lot of us rely on the code generation for our equals and hashcode methods. This works wonders and alleviates the need to write long methods, which is error-prone. Since Java 7, there are ways to write these methods in a more concise way, using Objects.equals and Objects.hashcode. There were already some libraries to do this (apache, guava), and some eclipse plugins that were handling this. But the great advantage here is that this is pure java! At the moment, it is not possible to write a custom template in Eclipse to have this generated properly, this requires a core change. http://stackoverflow.com/questions/17782032/how-to-teach-eclipse-to-generate-compact-equals-and-hashcode-from-the-jdk-7 Regards, Nicolas
I think this belongs to JDT UI, as a change in GenerateHashCodeEqualsDialog/SourceActionDialog is required.
Also GenerateHashCodeEqualsAction needs to be adjusted.
*** Bug 462484 has been marked as a duplicate of this bug. ***
Hello, Is anyone working on a fix for this bug? I'd like to help out with that.
> Is anyone working on a fix for this bug? I'd like to help out with that. Markus set the keywords "bugday, helpwanted" so I assume help is welcome. If you are new to Gerrit, you can use http://www.vogella.com/tutorials/EclipsePlatformDevelopment/article.html
(In reply to Lars Vogel from comment #5) > > Is anyone working on a fix for this bug? I'd like to help out with that. > > Markus set the keywords "bugday, helpwanted" so I assume help is welcome. If > you are new to Gerrit, you can use > http://www.vogella.com/tutorials/EclipsePlatformDevelopment/article.html Thanks!
New Gerrit change created: https://git.eclipse.org/r/81158
Hi, I uploaded an update on the Gerrit change, https://git.eclipse.org/r/81158 The build failed, with the build info page here: https://hudson.eclipse.org/platform/job/eclipse.jdt.ui-Gerrit/945/ I checked the build log, and also the error, it seems one particular test fails, which is org.eclipse.jdt.ui.tests.refactoring.ExtractConstantTests.test25 (from org.eclipse.jdt.ui.tests.refactoring.all.AllAllRefactoringTests) The actual test is this one: public void test25() throws Exception { helper1(5, 27, 5, 40, false, false, "DEFAULT_NAME", "JEAN_PIERRE"); } The console output from the build process ends with this, [ERROR] Failed to execute goal org.eclipse.tycho:tycho-surefire-plugin:0.25.0:test (default-test) on project org.eclipse.jdt.ui.tests.refactoring: There are test failures. I'm not sure what I'm supposed to do here, because I haven't touched this test case and it's not related to the change I've introduced. Also, I tested AllAllRefactoringTests locally and all tests were successful. I'd appreciate any help with this, thanks. Ivan
https://git.eclipse.org/r/#/c/81158/ Verification and build successful. Ivan
@Noopur, I'm not sure if you have seen that there is a gerrit by a contributor. There is a small merge conflict in GenerateHashCodeEqualsOperation caused by the change for bug 443146 which added TypeLocation-parameters to the ImportRewrite.addImport-calls. I've rebased it in my workspace and tried it. I've noted two things: - the setting "use 'instanceof' to compare types" isn't handled (that is 'instanceof' is always used when the option to use the java 7 methods is enabled) - the option should probably be disabled, if the project uses java 6 or earlier. So this still needs some work, but I don't think it is too late for 4.7
Thanks for taking a look, Till. @Ivan, could you please handle the cases mentioned by Till in comment #10 so that the patch is ready for review?
Hi @Noopur, Yes, I will look into it. Thanks, Ivan
Any idea when will this feature become available?
(In reply to Wenjie Zhang from comment #13) > Any idea when will this feature become available? It is currently targetted for 4.7. (In reply to Ivan Konstantinov from comment #12) > Yes, I will look into it. Ivan, I do not see any changes from comment #10 in patch set 10. Please add a comment when the patch is ready for review.
(In reply to Noopur Gupta from comment #14) > (In reply to Wenjie Zhang from comment #13) > > Any idea when will this feature become available? > > It is currently targetted for 4.7. > > (In reply to Ivan Konstantinov from comment #12) > > Yes, I will look into it. > > Ivan, I do not see any changes from comment #10 in patch set 10. Please add > a comment when the patch is ready for review. Hi, Patch set 11 is ready for review, I've implemented some checks as per comment #10.
Tests have failed, so the patch is not ready yet.
Patch set 13 is ready for review. Ivan
- Patch set 13 still doesn't handle this from comment #10: the option should be disabled if the project uses java 6 or earlier. - The generated #equals implementation results in NPE if the passed object is null. - Use import statement and simple name for java.util.Objects instead of hard-coding the FQN. - The #equals implementation still uses braces in at least one 'if' statement even when "Use blocks in 'if' statements" is not checked. - Recheck the new implementation of methods, keeping it similar to the existing style. - Please avoid code duplication.
Ivan, I just tested your updated patch set 15 for the first review comment in comment #18 about the option being disabled for projects using Java 6 or earlier, and it is still not handled. With a Java 5 project, the dialog doesn't show the new option at all and clicking OK still creates the methods using Objects methods resulting in compile errors. Moving this to 4.8.
(In reply to Noopur Gupta from comment #19) > Ivan, I just tested your updated patch set 15 for the first review comment > in comment #18 about the option being disabled for projects using Java 6 or > earlier, and it is still not handled. > > With a Java 5 project, the dialog doesn't show the new option at all and > clicking OK still creates the methods using Objects methods resulting in > compile errors. > > Moving this to 4.8. Interesting. When I tested this locally, including running all tests on GenerateHashCodeEqualsOperation, it seemed to work as desired, but now that I check, I get a different issue - with compliance level >= 1.7, I get the new option in the dialog but the resulting methods don't use Objects methods when checked. I'll check again.
Hi, Patch is ready for review. Ivan
(In reply to Ivan Konstantinov from comment #21) > Hi, > Patch is ready for review. > Ivan Sorry, we are currently very busy getting Java 9 and JUnit 5 support out. Please ping again if we haven't look at this until January.
(In reply to Dani Megert from comment #22) > Sorry, we are currently very busy getting Java 9 and JUnit 5 support out. > Please ping again if we haven't look at this until January. Putting M5 as deadline according to this comment. If someone feels able to work on this earlier and can help in getting it merged sooner, please change target version according to your availability.
Copying the comment from Gerrit: Ivan, can you please rebase this change manually and confirm if all the earlier review comments are handled and the patch is ready for review? Please request again for review when the patch is ready. Also, any help with an initial review will be appreciated.
(In reply to Noopur Gupta from comment #24) > Copying the comment from Gerrit: > Ivan, can you please rebase this change manually and confirm if all the > earlier review comments are handled and the patch is ready for review? > > Please request again for review when the patch is ready. > > Also, any help with an initial review will be appreciated. I'll have a look.
- primitives (like int) should not be compared with Object.equals, this causes autoboxing - mixing Objects.deepEquals(array, other.array) and Arrays.hashcode(array) for the same object is wrong, as Object[], Serializable[] and Cloneable[] can contain other arrays (the old code uses Arrays.equals(array) together with Arrays.hashcode(array) for these classes, which is at least consistent) - the old code generates: if (getClass() != obj.getClass()) return false; but the new code: if (getClass() == obj.getClass()) { ... } this is inconsistent and also leads to some duplicated code. - "java 7+" should probably be "1.7 or higher" to be consistent with other dialogs - I think if would be more consistent if the checkbox is always present, but disabled for older java, but this is less important
Ivan, are you still available to work on this patch? Sorry for the frequent delays in reviews but I think Till is really to act fast on any new fix you can provide. If you lost interest in this contribution, please let us know.
Moving it to M7.
(In reply to Till Brychcy from comment #26) I contact Ivan also via email and did not get a reply, I assume Ivan lost interest for this patch. Till, as you did already a review, could you maybe finish this patch?
Hello, I'm new to the Eclipse JDT UI project and to Gerrit, but I'm keen on helping out to get this feature added to Eclipse. I'll look into addressing the review comments listed by Till in the coming days/weeks.
(In reply to Pierre-Yves B. from comment #30) > Hello, > > I'm new to the Eclipse JDT UI project and to Gerrit, but I'm keen on helping > out to get this feature added to Eclipse. I'll look into addressing the > review comments listed by Till in the coming days/weeks. Good to hear. Assigning to you.
I'm pretty much done with the development work, but I'm having trouble submitting a new patch. I tried committing using the existing change ID ( I1b7258d69f3925b77b2f72f017afc64da7a0cac0), but the following message is displayed: "You must be a committer to push on behalf of others.", as Ivan initially submitted the Gerrit Change. I'm guessing we want to keep things under the same Gerrit Change, how should I proceed from this point onwards?
make sure you are set as author but add an also-by header for Ivan. If you still cannot push, use a fresh change Id.
Okay, that solved the issue. Till, I have hopefully addressed your five bullet points above. I have also added quite a few new unit tests and have refactored parts of the code written by Ivan to reduce duplication and tidy things up a little. Object[], Serializable[] and Cloneable[] are now compared with deepEquals/deepHashCode, and in order to stay consistent and avoid duplicating very similar pieces of code, this change covers the non Java 7 version of the generator as well.
Till, can you do the review?
(In reply to Dani Megert from comment #35) > Till, can you do the review? OK
(In reply to Pierre-Yves B. from comment #34) > Okay, that solved the issue. > > Till, I have hopefully addressed your five bullet points above. I have also > added quite a few new unit tests and have refactored parts of the code > written by Ivan to reduce duplication and tidy things up a little. > > Object[], Serializable[] and Cloneable[] are now compared with > deepEquals/deepHashCode, and in order to stay consistent and avoid > duplicating very similar pieces of code, this change covers the non Java 7 > version of the generator as well. Thanks, this looks good now. I'd only like to suggest a small improvement: use "getErasure" in "needsDeepMethod", so the deep methods are also used in the presence of type variables, e.g.: class X <T, S extends Serializable, N extends Number> { T[]ts; // should use deep methods S[]ss; // should use deep methods N[]ns; // should not use deep methods }
Thanks for reviewing Till. Good spot on the type variables, this is now covered by a test and should be fixed. Another minor bug was taken care of, we were importing "java.util.Objects" even if the class only contained arrays. Whilst I was at it I refactored some of the code around the hashCode generation which was responsible for that import. The CI build seems to have failed, but unless I'm missing something, it isn't related to my latest changes.
@Dani or @Lars, The Genie says the contribution is more than 1000 lines now and a CQ is needed. Can one of you please manage that?
(In reply to Pierre-Yves B. from comment #38) > Thanks for reviewing Till. Good spot on the type variables, this is now > covered by a test and should be fixed. > > Another minor bug was taken care of, we were importing "java.util.Objects" > even if the class only contained arrays. Whilst I was at it I refactored > some of the code around the hashCode generation which was responsible for > that import. Thanks. > > The CI build seems to have failed, but unless I'm missing something, it > isn't related to my latest changes. Yes, that was bug 537756
(In reply to Till Brychcy from comment #39) > @Dani or @Lars, The Genie says the contribution is more than 1000 lines now > and a CQ is needed. Can one of you please manage that? (I've replied to the Genie on the Gerrit as requested - not sure what else needs to be done.)
(In reply to Till Brychcy from comment #39) > @Dani or @Lars, The Genie says the contribution is more than 1000 lines now > and a CQ is needed. Can one of you please manage that? The person who looks at the code should also file the CQ. I did not look at the change, but if a bigger part is just code refactoring, you could separate that from the real fix.
(In reply to Dani Megert from comment #42) > (In reply to Till Brychcy from comment #39) > > @Dani or @Lars, The Genie says the contribution is more than 1000 lines now > > and a CQ is needed. Can one of you please manage that? > > The person who looks at the code should also file the CQ. I did not look at > the change, but if a bigger part is just code refactoring, you could > separate that from the real fix. I cannot, Genie doesn't let me.
BTW: If creating the CQ is a lot of work, we could either save some lines in the test code (current state is 1015, so 16 lines to be saved) or actually we could split it into two patches: Ivan's unfinished contribution and Pierre-Yves's completion.
(In reply to Till Brychcy from comment #43) > (In reply to Dani Megert from comment #42) > > (In reply to Till Brychcy from comment #39) > > > @Dani or @Lars, The Genie says the contribution is more than 1000 lines now > > > and a CQ is needed. Can one of you please manage that? > > > > The person who looks at the code should also file the CQ. I did not look at > > the change, but if a bigger part is just code refactoring, you could > > separate that from the real fix. > > I cannot, Genie doesn't let me. You are a committer on JDT, so, that should work. How did you try to create it? What does it say? Usually you have to go to the project on the portal, e.g. https://projects.eclipse.org/projects/eclipse.jdt then click on 'Create a Contribution Questionnai...' (you have to be logged in).
> Usually you have to go to the project on the portal, e.g. > https://projects.eclipse.org/projects/eclipse.jdt > then click on 'Create a Contribution Questionnai...' (you have to be logged > in). I tried by replying to the Genie on the Gerrit as it suggested. It says: > Received CQ creation request but reviewer is not a committer on > this project. A project committer must comment genie:cq to trigger > CQ creation. Probably this didn't get updated when jdt projects were merged?
Dani, is creating the CQ a lot of work? (I've never done it.) Do you recomment getting the patch size below 1000 lines instead?
(In reply to Till Brychcy from comment #47) > Dani, is creating the CQ a lot of work? (I've never done it.) > Do you recomment getting the patch size below 1000 lines instead? Not for you, but for the IP team. Also, if I recall correctly, we are already past the CQ deadline for 4.9.
(In reply to Dani Megert from comment #48) > (In reply to Till Brychcy from comment #47) > > Do you recomment getting the patch size below 1000 lines instead? > > Not for you, but for the IP team. Also, if I recall correctly, we are > already past the CQ deadline for 4.9. I'm interpreting that as a yes (and I meant for everybody, not only for me).
I'll trim down the contribution in the evening. There's some repetition in the tests, we can easily make them slightly slimmer.
(In reply to Pierre-Yves B. from comment #50) > I'll trim down the contribution in the evening. There's some repetition in > the tests, we can easily make them slightly slimmer. Thanks. There are some changes in the import order that can be avoided and many old tests are changed to have a "false"-parameter added. Maybe you could add just another overload of runOperation that provides that extra false-argument, then the old tests don't have to be changed.
(In reply to Till Brychcy from comment #46) > I tried by replying to the Genie on the Gerrit as it suggested. It says: > [...] I've created bug 537783 for this.
Down to 996 new lines of code, Genie should now be happier!
Gerrit change https://git.eclipse.org/r/81158 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=f543cd6d9e9f81013404c079cb4b18cc0601ad44
(In reply to Eclipse Genie from comment #54) > Gerrit change https://git.eclipse.org/r/81158 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/ > ?id=f543cd6d9e9f81013404c079cb4b18cc0601ad44 Released for 4.9M3 One thing remains: Can you please provide an entry for the "New & Noteworthy"-page? The git repository is git://git.eclipse.org/gitroot/www.eclipse.org/eclipse/news.git and the entry needs to be added to 4.9/jdt.html Please note instructions.html especially w.r.t. the image width and html validation.
Note the latest integration build which already contains this can be downloaded from http://download.eclipse.org/eclipse/downloads/
Awesome, thanks Till! I will work on the "New & Noteworthy" entry.
New Gerrit change created: https://git.eclipse.org/r/127262
Gerrit change https://git.eclipse.org/r/127262 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=ad8d4511ea2c2e7df23dad14efa7cfb6f6df6f26
(In reply to Eclipse Genie from comment #59) > Gerrit change https://git.eclipse.org/r/127262 was merged to [master]. > Commit: > http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/ > ?id=ad8d4511ea2c2e7df23dad14efa7cfb6f6df6f26 I've corrected the placement of the "..." and added a .txt file corresponding to the image. I'm not sure if a new section was really necessary (wouldn't have felt wrong in "Java Views and Dialogs" to me), but I've committed it like this for now. You can already look at it at https://www.eclipse.org/eclipse/news/4.9/jdt.php#JavaCodeGeneration Thanks a lot Pierre-Yves and Ivan!
Thanks for your quick reviews and feedback Till, this was an interesting piece of work and it will hopefully prove useful to many users!
Verified by build I20180820-0800
The new label has no mnemonic. Filed bug 538273 to get this fixed.
