Community
Participate
Working Groups
ConcurrentModificationException on foreach loops org.eclipse.jdt.internal.ui.typehierarchy.TypeHierarchyViewPart:385 org.eclipse.jdt.internal.ui.text.java.hover.BestMatchHover:77 This is a revert from "Bug 548309 - Use jdk 5 for-each loop"
New Gerrit change created: https://git.eclipse.org/r/163558
(In reply to Fabrice Tiercelin from comment #0) > ConcurrentModificationException on foreach loops > org.eclipse.jdt.internal.ui.typehierarchy.TypeHierarchyViewPart:385 > org.eclipse.jdt.internal.ui.text.java.hover.BestMatchHover:77 > > This is a revert from "Bug 548309 - Use jdk 5 for-each loop" Please point to the exact patch we are talking here. This crazy bug has gazillions of patches.
Noopur, unfortunately I have no time to follow up here right now (have critical regression internally), but it looks like Sebastian found more worrying "optimization" changes. Could you please check which change broke what and may be revert them all? Original change (that caused bug 563532) and where I've asked to check for other regressions was this one: https://git.eclipse.org/r/#/c/158156/ but looks like the comment 0 means another patch: https://git.eclipse.org/r/#/c/144555/ which is also the one Sebastian have expressed concerns.
I cannot pinpoint to a specific change yet that caused the CME in TypeHierarchyViewPart. I'm in favour of reverting the entire thing.
(In reply to Sebastian Zarnekow from comment #4) > I cannot pinpoint to a specific change yet that caused the CME in > TypeHierarchyViewPart. I'm in favour of reverting the entire thing. With "thing" you mean https://git.eclipse.org/r/#/c/144555/?
Exactly.
How did you observe those ConcurrentModificationException in comment #0? They are probably caused by older multi-threading issues and not related to the for-loop conversion at all. I'm sceptical of most "cleanups", as e.g. if often find a lambda more readable than a method reference, but enhanced for-loops almost always make the code more readable, so I'm not in favor for a blind revert in this case. But of course I think a committer must manually inspect every single changed code location after applying a cleanup. Can you confirm that this was done for all those changes?
Till, I agree with what you are saying but even eye-balling all the changes does not guarantee that nothing slips through the cracks. After taking another look at the change in BestMatchHover, I agree that the CME is not caused by the change but would have happend with the old code, too (https://github.com/eclipse/eclipse.jdt.ui/commit/a1f5fa062403b3348a75ae63b17446f0011715a3#diff-8e6e8d23aabe00d24f311dd8c9d342cb).
(In reply to Sebastian Zarnekow from comment #6) > Exactly. Unfortunately a naive attempt to git revert 6d8ddd1100878431af0dec2418e2d0a67af51d48 results in a merge conflict on 4 files below, with lot of changes org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/wizards/buildpaths/AddSourceFolderWizardPage.java org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/wizards/buildpaths/LibrariesWorkbookPage.java org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/workingsets/ConfigureWorkingSetAssignementAction.java org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/workingsets/DynamicSourcesWorkingSetUpdater.java Looks like lot of related "optimization" commits must be reverted first, commit 3c9156b4387230cf7d99a6b0927392f0baaaf178 (change https://git.eclipse.org/r/#/c/160225/), commit 65c477a38a27f0a6df5057824a1f5522efc2c2b6 (https://git.eclipse.org/r/#/c/158159/), commit 49c611431cd6f60abfc6e55d67aa21ae7d1dcc31 (change https://git.eclipse.org/r/#/c/158160/) but still we have merge collisions. I give up.
After discussing with others in the team and Dani: It is decided that we will not revert all the commits from bug 548309 for 4.16 as it will have many revert/merge conflicts and can cause more regressions. For 4.16 RC1, we need to revert any commit: (1) that was released after M3 without approval from a JDT lead, and, (2) the ones that are now known to cause problems. Respective owners and reviewers of the commits from bug 548309, please do the following: (1) Provide separate revert patches for such commits via this bug that need to be reverted for 4.16 RC1 today. (2) Go through all the commits released via bug 548309 and provide reverts or fixes for the ones that can cause potential issues for 4.17 M1.
Thanks, Andrey, Sebastian, and Till for investigating the possible commits causing the issues. I see comments to check and revert the commits in multiple Gerrits. It will help if you can list such Gerrits that you are aware of to be reverted for RC1. The commit owners and reviewers should provide a revert for them in RC1.
I can confirm all the wrong commits have been reverted. Here is the list of commits to revert with their respective revert gerrit: c055e24f9541ab7a13fd2b9e4984a5ed28fb14c3 Convert for loop into enhanced loop, even if the iterator is a raw type https://git.eclipse.org/r/163510 245ce1ca31cac6dd28ce848490ed61cd85c5ef5d Bug 563315 - [extract method] Clean all useless parenthesis https://git.eclipse.org/r/163574 18951b92faae80966a96e21e07af99ff2def6531 Bug 563317 - [cleanup] Convert iterable loop: Loose code matching https://git.eclipse.org/r/163575
Everything has been restored and the situation is back to normal now.
@Fabrice, anything pending in this bug? If not, please close it for M1.
I have the new implementation but I'm trying to add a test case.
New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/167419
Ping!
(In reply to Noopur Gupta from comment #17) > Ping! I have updated the patch to address the last comment. I will +1 it after it verifies but it will need your permission to merge for RC1 today. https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/163558
(In reply to Jeff Johnston from comment #18) > (In reply to Noopur Gupta from comment #17) > > Ping! > > I have updated the patch to address the last comment. I will +1 it after it > verifies but it will need your permission to merge for RC1 today. > > https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/163558 Thanks, Jeff. +1 for RC1.
Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/163558 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=76080133bfaa384a2af93f1b49c560d84a5c16db
Verified for 4.17M3 using I20200819-0600 build
