Community
Participate
Working Groups
The method in the bug title was involved in intermittent failures in org.eclipse.jdt.core.tests.dom.RunAllTests when run against Object Teams. The results come at three steps: 1) The method has an off-by-one bug which, however, doesn't cause harm, normally. 2) The method may trigger what looks like a bug in Oracle's VM 1.7.0-b144 3) The method doesn't seem to be needed in current code. Details: (1) Inside removeTrailingCommentFromExpressionEndingWithAParen() the scanner is repositioned using: this.scanner.resetTo(start, start + node.getLength()); so for this test snippet: System.out.println(new Object()); the scanner now sees: new Object()) While the method is intended for reducing the source range of 'node' the scanner range theoretically allows also extending the source range which is not intended. As said, normally this causes no harm as matching of parens gives the correct result. (2) The failures I saw never happened when running in the debugger. So I did some sysout-debugging and observed that in these lines: case TerminalTokens.TokenNameRPAREN : parenCounter--; if (parenCounter == 0) { int end = this.scanner.currentPosition - 1; parenCounter was decremented to -1 and immediately after that the following if-then was entered! I just started seeing this and I assume this is because I just now switched to running these tests on a 1.7 VM (1.7.0-b144 to be precise). Later I installed 1.7.0-b147 (final) and couldn't yet reproduce on that VM, so maybe it's fixed, maybe it just happens less frequently (it never happened very deterministically). (3) Then I searched why removeTrailingCommentFromExpressionEndingWithAParen() was introduced in the first place, found bug 10874, removed the method body and ran all dom tests again: no failure. Specifically, ASTConverterTest has a number of tests that mention bug 10874, but none of these tests is effected by emptying removeTrailingCommentFromExpressionEndingWithAParen() Should we remove this method altogether, or can anybody demonstrate that it's still needed?
I'm running all tests to see if any lead to a call to this method.
Found one test atleast - org.eclipse.jdt.core.tests.dom.StandAloneASTParserTest.test3(). Looking at the references for the method, I can see that it is used for converting MessageSend and AllocationExpression Stephan, assuming this is indeed used, I think the only issue we should fix here is (1)?
(In reply to comment #2) > Found one test atleast - > org.eclipse.jdt.core.tests.dom.StandAloneASTParserTest.test3(). > > Looking at the references for the method, I can see that it is used for > converting MessageSend and AllocationExpression It's called, but I don't see any behavior changes if I delete the method. So, why keep it? What breaks?
(In reply to comment #0) > Details: > (1) > Inside removeTrailingCommentFromExpressionEndingWithAParen() the scanner > is repositioned using: > this.scanner.resetTo(start, start + node.getLength()); > so for this test snippet: > System.out.println(new Object()); > the scanner now sees: > new Object()) > While the method is intended for reducing the source range of 'node' > the scanner range theoretically allows also extending the source range > which is not intended. > As said, normally this causes no harm as matching of parens gives the > correct result. This is probably not expected. > (2) > The failures I saw never happened when running in the debugger. So I did > some sysout-debugging and observed that in these lines: > case TerminalTokens.TokenNameRPAREN : > parenCounter--; > if (parenCounter == 0) { > int end = this.scanner.currentPosition - 1; > parenCounter was decremented to -1 and immediately after that > the following if-then was entered! > I just started seeing this and I assume this is because I just now switched > to running these tests on a 1.7 VM (1.7.0-b144 to be precise). > Later I installed 1.7.0-b147 (final) and couldn't yet reproduce on that VM, > so maybe it's fixed, maybe it just happens less frequently (it never happened > very deterministically) There was a JIT bug that has been fixed with b147. > (3) > Then I searched why removeTrailingCommentFromExpressionEndingWithAParen() > was introduced in the first place, found bug 10874, removed the method body > and ran all dom tests again: no failure. Specifically, ASTConverterTest > has a number of tests that mention bug 10874, but none of these tests is > effected by emptying removeTrailingCommentFromExpressionEndingWithAParen() > Should we remove this method altogether, or can anybody demonstrate that > it's still needed? If we cannot come up a test case that is passing through that code, we should get rid of it.
I'll take care of this.
The code seems to be useless as the positions are properly set before the code is called. This might be left overs from a time where the end position was wrong. I would simply get rid of it.
Released for 3.8M2. Verification needs to be done by code inspection.
Verified for 3.8M2 using code inspection
