Community
Participate
Working Groups
This exception happens in 4.5RC2 but not in 4.4.2. java.lang.NullPointerException at org.eclipse.jdt.internal.compiler.Compiler.restoreAptProblems(Compiler.java:514) at org.eclipse.jdt.internal.compiler.Compiler.compile(Compiler.java:457) at org.eclipse.jdt.internal.compiler.Compiler.compile(Compiler.java:452) at org.eclipse.jdt.internal.core.builder.AbstractImageBuilder.compile(AbstractImageBuilder.java:367) at org.eclipse.jdt.internal.core.builder.BatchImageBuilder.compile(BatchImageBuilder.java:179) at org.eclipse.jdt.internal.core.builder.AbstractImageBuilder.compile(AbstractImageBuilder.java:304) at org.eclipse.jdt.internal.core.builder.BatchImageBuilder.build(BatchImageBuilder.java:61) at org.eclipse.jdt.internal.core.builder.JavaBuilder.buildAll(JavaBuilder.java:256) at org.eclipse.jdt.internal.core.builder.JavaBuilder.build(JavaBuilder.java:175) at org.eclipse.core.internal.events.BuildManager$2.run(BuildManager.java:734) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:205) at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:329) at org.eclipse.core.internal.events.BuildManager.build(BuildManager.java:404) at org.eclipse.core.internal.resources.Project$1.run(Project.java:556) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2241) at org.eclipse.core.internal.resources.Project.internalBuild(Project.java:534) at org.eclipse.core.internal.resources.Project.build(Project.java:119) Unfortunately, I don't have a self-contained example that triggers this problem.
New Gerrit change created: https://git.eclipse.org/r/49014
I'm having a hard time understanding the Gerrit review format. Is the "for (CompilationUnitDeclaration unitDecl : this.unitsToProcess)" version the new one, or the old one? If it's the new one, it looks good. If it's the old one, I don't understand the change.
(In reply to Walter Harley from comment #2) > I'm having a hard time understanding the Gerrit review format. Is the "for > (CompilationUnitDeclaration unitDecl : this.unitsToProcess)" version the new > one, or the old one? > > If it's the new one, it looks good. If it's the old one, I don't understand > the change. The modified code is on the right. The unitsToProcess array contains valid data only in the first totalUnits elements.
I see. The reason this code has had so many bugs over its history is because it violates separation of concerns and it violates principle of least surprise. This bug is an example of that: the management of the array (the logical coupling of unitsToProcess and totalUnits, and the array-growing algorithm) is mixed up with the use of the array. Presented with an array it's reasonable to think that it's safe to iterate over the whole array, but in this case it's not. So you end up reintroducing iteration with i++, which opens the door to off by one errors and the like. This code would be a lot cleaner if the array data structure was a separate class. And at this point I really wonder whether managing our own array growing and shrinking is buying us any performance. Grumble. Anyway, I guess in terms of actual bugs, the change seems correct as far as I can tell.
Sorry I was away on leave during the last week. It would be good to know the scenario causing this so we can evaluate including this for Mars, although we are already in RC4.
(In reply to Jay Arthanareeswaran from comment #5) > Sorry I was away on leave during the last week. It would be good to know the > scenario causing this so we can evaluate including this for Mars, although > we are already in RC4. Or a test case. The proposed code change itself looks good to me.
(In reply to Jay Arthanareeswaran from comment #5) > Sorry I was away on leave during the last week. It would be good to know the > scenario causing this so we can evaluate including this for Mars, although > we are already in RC4. Unfortunately, I don't have a self contained reproduction case. Still it is a no-brainer that the change should be included in Mars. It's pretty obvious that the code before the change is incorrect and requires modification.
(In reply to Sergey Prigogin from comment #7) > Still it is > a no-brainer that the change should be included in Mars. It's pretty obvious > that the code before the change is incorrect and requires modification. No doubt, it just that it makes it easier to get approvals when we have a reproducible test case. I agree with the proposed changes.
+1 for RC4. The code change looks safe, and I've added a test case to the review. Without the fix, the test fails with the given NPE. Filed bug 469031 for a SourceTypeCollisionException that escapes from the compiler when I modify the test a bit.
(In reply to Dani Megert from comment #6) > (In reply to Jay Arthanareeswaran from comment #5) > > Sorry I was away on leave during the last week. It would be good to know the > > scenario causing this so we can evaluate including this for Mars, although > > we are already in RC4. > > Or a test case. > > The proposed code change itself looks good to me. +1.
Gerrit change https://git.eclipse.org/r/49014 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=7a6bf774efceb93e4277a70cc59614d5d931a27e
Submitted.
Verified for 4.5 RC4 with build I20150601-2000. Sergey, I encourage you to grab this build and confirm it works in your environment too.
(In reply to Jay Arthanareeswaran from comment #13) Confirmed.
