Review: feat(position): add the position of the modifier in the ast #1959
Conversation
13dce20 to
af26c52
Compare
| @@ -53,6 +55,14 @@ public void setKind(ModifierKind kind) { | |||
| this.kind = kind; | |||
| } | |||
|
|
|||
| public SourcePosition getPosition() { | |||
| return position; | |||
There was a problem hiding this comment.
if (position == null) return NoSourcePosition;
WDYT? CtElement already never returns null.
af26c52 to
4f5cdb4
Compare
|
|
||
| Set<CtExtendedModifier> modifiers = e.getExtendedModifiers(); | ||
| if (start < 0 || end + 1 > contents.length) { | ||
| System.out.println(e); |
There was a problem hiding this comment.
Is it really wanted?
There was a problem hiding this comment.
sorry, debug point that I forget to remove
| modifiersSourceStart = declarationSourceStart; | ||
| } | ||
| //look for start of first keyword before the type keyword e.g. "class". `sourceStart` points at first char of type name | ||
| int modifiersSourceEnd = findPrevNonWhitespace(contents, modifiersSourceStart - 1, | ||
| findPrevWhitespace(contents, modifiersSourceStart - 1, | ||
| findPrevNonWhitespace(contents, modifiersSourceStart - 1, sourceStart - 1))); | ||
| if (e instanceof CtModifiable) { | ||
| setModifiersPosition((CtModifiable) e, modifiersSourceStart, bodyStart); |
There was a problem hiding this comment.
I have not read whole code yet, so may be it is correct, but usage of bodyStart here seems to be wrong.
There was a problem hiding this comment.
ok, the searched string is little bit longer then necessary, but I cannot imagine code where it would cause problem, so it is OK for me.
There was a problem hiding this comment.
We are here in a TypeDeclaration the bodyStart is the first position that I'm sure will contain the modifier.
I'm pretty sure that the way to perform modifiersSourceEnd in this case does not support annotation and thus can be incorrect in some clases
| if (start < 0 || end + 1 > contents.length) { | ||
| System.out.println(e); | ||
| } | ||
| String modifierContent = String.valueOf(Arrays.copyOfRange(contents, start, end + 1)); |
There was a problem hiding this comment.
I guess there is faster new String(contents, start, end+1) or something like that. Or why do we copy that array?
|
It looks good for me. Thank you for these fixes! Are you finished @tdurieux ? Note: the searching for location of modifiers should count with comments, @Deprecated /* public */ public class Wild {}but it is wild case and we can fix it later - when it causes real problem somewhere. |
|
I thought about this and also in an annotation contains public or final or static but handle them is not that easy and it is probably pretty rare. |
|
API changes: 1 (Detected by Revapi) Old API: fr.inria.gforge.spoon:spoon-core:jar:6.3.0-20180405.225214-41 / New API: fr.inria.gforge.spoon:spoon-core:jar:6.3.0-SNAPSHOT
|
No description provided.