review: feat: java lexer for better position detection #5753
Conversation
SirYwell
changed the title
|
I had a nightmare that @SirYwell was writing a complete lexer. Luckily, this did not happen or? |
| contents, | ||
| start, | ||
| Math.max(start, end) + 1, //move end after the last char | ||
| explicitModifiersByKind, | ||
| (modStart, modEnd) -> cf.createSourcePosition(cu, modStart, modEnd, cu.getLineSeparatorPositions()) |
There was a problem hiding this comment.
| contents, | |
| start, | |
| Math.max(start, end) + 1, //move end after the last char | |
| explicitModifiersByKind, | |
| (modStart, modEnd) -> cf.createSourcePosition(cu, modStart, modEnd, cu.getLineSeparatorPositions()) | |
| contents, | |
| start, | |
| Math.max(start, end) + 1, //move end after the last char | |
| explicitModifiersByKind, | |
| (modStart, modEnd) -> cf.createSourcePosition(cu, modStart, modEnd, cu.getLineSeparatorPositions()) |
and why is there a Math.max? Is end ever smaller than start? I think this either deserves an explanation or should be end + 1.
There was a problem hiding this comment.
For some unholy reason, end can be -1 sometimes. That can probably be fixed, but I don't want to include that here.
| i += 5; | ||
| if (this.positionRemap == null) { | ||
| this.positionRemap = createPositionRemap(chars); | ||
| } |
There was a problem hiding this comment.
Could you add a comment stating why this is 6? And maybe a short comment at the top saying that you are first building a map from index -> skip value to next char and then accumulate it at the bottom or something in that spirit?
| if (this.content[i] == '\\') { | ||
| if (escape) { | ||
| escape = false; | ||
| } else if (this.end > i + 1 && this.content[i + 1] == '\\') { | ||
| escape = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
I am not quite sure what this is doing. Maybe rename escape to escapeBackslash or just handle it inline and increment i by one in that branch and skip the loop iteration? At present I am not exactly sure what this is doing.
| package spoon.support.util.internal.lexer; | ||
|
|
||
| /** | ||
| * Valid Java (contextual) keywords |
There was a problem hiding this comment.
But VAR for example is missing, so not all contextual?
| readHexadecimalsAndUnderscore(); | ||
| } | ||
| } else if (peek() == 'b' || peek() == 'B') { | ||
| next(); |
There was a problem hiding this comment.
| next(); | |
| // binary literal | |
| next(); |
| // check if hexadecimal notation | ||
| if (first == '0') { | ||
| if (peek() == 'x' || peek() == 'X') { | ||
| next(); |
There was a problem hiding this comment.
| next(); | |
| // hexadecimal literal | |
| next(); |
| readDigitsOrUnderscore(); | ||
| } | ||
| } else { | ||
| readDigitsOrUnderscore(); |
There was a problem hiding this comment.
| readDigitsOrUnderscore(); | |
| // decimal literal | |
| readDigitsOrUnderscore(); |
?
| return false; | ||
| } | ||
|
|
||
| private void readNumericLiteral(char first) { |
There was a problem hiding this comment.
How does this lex 1e20f? Can you add a test for that?
|
@SirYwell Can you have a look at the remaining comments? |
Fixes #4705.
As mentioned in the linked PR, the current approach to find source locations in spoon is insufficient. The naive approach on only caring about spaces is not enough as there are other separators. By implementing a lexer, we can deal with that properly and also support unicode sequences.
This solution will hopefully - besides correctness - be more future-proof.
I tested the lexer on all
.javafiles of the OpenJDK. By manually inspecting failures, it looks like only actual invalid code is rejected. So I'm confident this is a robust implementation.There is more code in the PositionBuilder relying on the whitespace-based methods. It might be possible to replace those usages in future too.