Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 409250

Summary: [1.8][compiler] Various loose ends in 308 code generation.
Product: [Eclipse Project] JDT Reporter: Srikanth Sankaran <srikanth_sankaran>
Component: CoreAssignee: Andrew Clement <aclement>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 Flags: srikanth_sankaran: review+
Version: 4.3   
Target Milestone: BETA J8   
Hardware: PC   
OS: Windows 7   
Whiteboard:
Bug Depends on:    
Bug Blocks: 409235    
Attachments:
Description Flags
patch to address the issues here none

Description Srikanth Sankaran CLA 2013-05-28 04:18:10 EDT 
BETA_JAVA8:

This bug will be used to cover the loose ends discovered in code review
of the core implementation of JSR308 code generation.

(1) Should TypeAnnotationCodeStream.invoke(byte, MethodBinding, TypeBinding, 
TypeReference[]) be checking for ASTNode.HasTypeAnnotations ?

(2) Is ExtendedAnnotation violating javadoc of 
IExtendedAnnotation.getLocalVariableTable() ?

(3)  CodeStream(s): Is the method CodeStream.newArray(TypeReference, 
ArrayBinding) needed ? 

(4) ClassFile.addFieldAttributes: Needs a relook. When a field declaration 
carries a type annotation (or for that matter when a type annotation occurs in 
any place where a Java SE5 annotation can occur, we leave it annotating the 
declared entity - i.e the type annotation is not moved to the type.) So code 
fragments like if (fieldType.bits & ASTNode.HasTypeAnnotations) != 0) may not 
get executed at all. Until the resolve phase, we don't know whether an 
annotation is a type declaration and so the HasTypeAnnotations bit cannot be set 
in these places.

For concern 4, Andy's initial response is available at

https://bugs.eclipse.org/bugs/show_bug.cgi?id=383624#c54

I feel the suggestion to set the bit only during resolution is
a good one.
Comment 1 Srikanth Sankaran CLA 2013-08-16 08:41:24 EDT 
I just commented on https://bugs.eclipse.org/bugs/show_bug.cgi?id=414384
that that is the only open issue against your name - Andy, please also
take up this one. Thanks!

If these two are resolved, we are ready to declare the 308 code generation project (https://bugs.eclipse.org/bugs/show_bug.cgi?id=409235) completed.
Comment 2 Andrew Clement CLA 2013-08-23 16:10:06 EDT 
> (1) Should TypeAnnotationCodeStream.invoke(byte, MethodBinding, TypeBinding, 
TypeReference[]) be checking for ASTNode.HasTypeAnnotations ?

Checking it feels like a nice cheap check before going looking for something that may or may not be there. Or do you think something more untoward is happening?

I see the TODO on that line says can we do the test at a higher level?  We can, but it does make a bit of a mess of the codebase.  Requiring the caller to discover/remember whether any of the type arguments have type annotations and pass that information around in addition to the type arguments. I am inclined to leave it as it is.

But really the reason to change any of this is if we think the extra overhead of iterating through type arguments in the invoke() method is too costly. Given that most methods don't have type arguments that code just won't run anyway, in the normal case.

> (2) Is ExtendedAnnotation violating javadoc of IExtendedAnnotation.getLocalVariableTable() ?

I guess this refers to the fact that it might return a null object when the javadoc says it will return an empty array.  This should be tidied up, yes, but in practice it won't go wrong right now as nobody asks for the local variable table unless they are expecting it to be there at the moment.

> (3)  CodeStream(s): Is the method CodeStream.newArray(TypeReference, 
ArrayBinding) needed ? 

No, we can ditch it.

> (4) ClassFile.addFieldAttributes: Needs a relook. 

Ok, I can switch it to setting the bit during resolution.
Comment 3 Andrew Clement CLA 2013-08-23 16:12:26 EDT 
Created attachment 234718 [details]
patch to address the issues here

This includes the changes I mention in the previous comment

(1) removed TODO
(2) creates an empty array by default that will be used.  Could set it empty on reference but I just went this way...
(3) Unnecessary newArray removed
(4) parser setting of bits removed. resolution steps in local/field declarations now setting the bits.
Comment 4 Srikanth Sankaran CLA 2013-08-25 08:02:29 EDT 
For some reason, your patches don't apply clean when I paste them on the
package explorer. The problem seems to be always with the last file, I get
by applying the changes manually.

Fix looks good: Released here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=e965722f47023ae407b487744865b93f56cfe7d1.

Thanks Andy