Community
Participate
Working Groups
BETA_JAVA8: For a variable arity method, the compiler AST contains an extra dimension while on the DOM/AST side the Type of SingleVariableDeclaration does not contain an extra dimension, but captures the fact of variable arity in a boolean property: VARARGS_PROPERTY. So SingleVariableDeclaration needs to be extended to capture, expose and allow manipulations on the annotations on the ...
Created attachment 222879 [details] Proposed Patch - Work-in-progress. This is a work-in-progress patch - please provide early review comments. - ASTConverter delta needs to be checked specifically. - Also, the point discussed in https://bugs.eclipse.org/bugs/show_bug.cgi?id=391895#c2 needs to be discussed for incorporating into this fix if required.
(In reply to comment #1) > Created attachment 222879 [details] > Proposed Patch - Work-in-progress. Questions/points: 1. Even in the case of variable args, the argument (i.e., the SingleVaribaleDeclration) can have annotation along with annotations on dimenstions. So, we need to come up with a new field for that rather than using the existing annotations field. For e.g., in this case - foo(@Marker1 int @Marker2 ... args){} The SVD should get the @Marker1 (in an array, of course) and @Marker2 should go to the annotations on dimensions. Even if you argue that for a varargs parameter the annotation on the parameter doesn't make sense (which I don't agree with), we have to have a separate field for the annotations on dimensions. We can't use the same field for dual purpose to avoid confusion. 2. Instead of cloning the annotations from the type and setting them on dimensions, we should try to avoid the cloning part. Try to set it at the right place the first time itself. 3. This question is for Srikanth: Why can't treat the varargs case like a parameter of array type and that will take care of most of the annotation related tasks. I know the API are already there and such. But if we just convert the varargs parameter to an SVD with it's type as an ArrayType of single dimension, we don't need any specific code for var args case. I mean, the ArrayType will anyway get it's annotation on dimensions won't it?
(In reply to comment #2) > Questions/points: > 1. Even in the case of variable args, the argument (i.e., the > SingleVaribaleDeclration) can have annotation along with annotations on > dimenstions. So, we need to come up with a new field for that rather than > using the existing annotations field. > > For e.g., in this case - foo(@Marker1 int @Marker2 ... args){} > The SVD should get the @Marker1 (in an array, of course) and @Marker2 should > go to the annotations on dimensions. Please, ignore this question. I must have known that we do support annotations on SVD already. What I didn't know was, until Srikanth told me, that the modifiers2 field is used to capture annotations. So, what we really need is a specific field to capture the annotations on the varargs' dimensions.
Created attachment 223159 [details] Proposed Patch - Work-in-progress. Todo: - Add more tests [particularly matcher tests] - Confirming functionality based on Varargs [as part of review?] - Review by Manoj for functionality issues and for patch review c/l adherence.
Created attachment 223210 [details] Proposed Patch - Work-in-progress. Todo: - Rewrite/Correct the existing test cases that are failing due to new syntax of type annotations. - More review and testing by Manoj.
On a rebase, tests pass. This same patch may be reviewed as a fix.
Created attachment 223997 [details] Non ASTRewrite Part of the original patch The earlier patch has been now being uploaded as two parts for ease of review : NonASTRewrite Part and ASTRewrite Part. This attachment is NonASTRewrite Part
Created attachment 223999 [details] ASTRewrite Part of the original Patch ASTRewrite Part of the Patch
(In reply to comment #7) > Created attachment 223997 [details] > Non ASTRewrite Part of the original patch > > The earlier patch has been now being uploaded as two parts for ease of > review : NonASTRewrite Part and ASTRewrite Part. This attachment is > NonASTRewrite Part Here are some review comments: (1) SVD: The javadoc update (diff 2 in SVD) is incorrect, looks identical to JLS3 change (i.e missing optional annotations) (2) SVD: ANNOTATIONS_PROPERTY is better named VARARGS_ANNOTATIONS_PROPERTY ? SVD itself can have annotations which are captured in modifiers. (3) SVD: javadoc for the new property should also NOT say : The "annotations" structural property of this node type ... (4) SVD: I believe the CYCLE_RISK parameter should actually be NO_CYCLE_RISK. Can annotations contains a single variable declarations ? (5) SVD field annotations is better named varargsAnnotations, does this have to be protected ? (6) SVD: annotations() is better named varargsAnnotations() ? The javadoc above also needs fixing, it is too general and confusing : "Returns the live ordered list of annotations for this SingleVariableDeclaration node" (7) NaiveASTFlattener: Is there a reason why the call to this.buffer.append(' '); happens before as opposed to after the annotations gets visited ? Compare visitTypeAnnotations methods in the same file. We probably need it both before and after (including in old places ?)
Created attachment 224260 [details] Proposed Patch - NONASTRewrite Part - Reworked
(In reply to comment #10) > Created attachment 224260 [details] > Proposed Patch - NONASTRewrite Part - Reworked 1. SingleVariableDeclaration grammatical notation includes a [] pair incorrectly. 2. Sorry if this was not clear in earlier comment 9 point 2. The new property's id should not be "annotations" - it should be varargsAnnotations. Likewise the javadoc needs to be suitably adjusted. 3. NaiveASTFlattener: Won't there be two white spaces after the last annotation ? Jay, After these are fixed, please quickly eyeball the patch and release it - Thanks!
Created attachment 224702 [details] Proposed Patch [after rework]
(In reply to comment #12) > Created attachment 224702 [details] > Proposed Patch [after rework] 1. The property name in the Javadoc has a typo - it should be varargsAnnotations and not varargsannotations 2. The Javadoc for varargsAnnotations field doesn't mention varargs. 3. In the SVD's Javadoc, I see 'varargs'. Consider using the full form if it makes sense. 4. May be we can avoid the clone of the annotations. The ArrayType that gets created with the type annotations is discarded anyway. So, if we can find a way to avoid adding annotations to the ArrayType in case of varargs, we can do it at a later point of time when required.
Created attachment 225128 [details] Proposed Patched - [Reworked]
Created attachment 225129 [details] Proposed Patch [Reworked] Resubmitting the patch obsoleting earlier ones.
Patch is good and released to BETA_JAVA8: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=3388256fa80bf40daffa9f485d2674986c875b82
