Summary: | [dom] ASTConverter.removeTrailingCommentFromExpressionEndingWithAParen() ?? | ||
---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Stephan Herrmann <stephan.herrmann> |
Component: | Core | Assignee: | Olivier Thomann <Olivier_Thomann> |
Status: | VERIFIED FIXED | QA Contact: | |
Severity: | minor | ||
Priority: | P3 | CC: | amj87.iitr, Olivier_Thomann |
Version: | 3.7.1 | ||
Target Milestone: | 3.8 M2 | ||
Hardware: | PC | ||
OS: | Linux | ||
Whiteboard: |
Description
Stephan Herrmann
2011-08-06 16:10:46 EDT
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 |