Better encapsulation for AnnotatedType

[Werner Dietl]

Hi Joel, all,

thanks for the comments.

I think the only tricky part in this refactoring was that in
Attr.annotateType(...) instead of modifying the Type and adding the
type annotations, I'm now setting the type of the tree to the new
AnnotatedType:

tree.type = tree.type.unannotatedType().annotatedType(compounds);

This means that the tree type appears without type annotations until
the annotate.typeAnnotation queue is emptied. This shouldn't make a
difference, as in the old version one would have seen an AnnotatedType
with empty annotations - which is also unexpected.

Please do let me know whether I should look into TypeMetadata or
whether one of you is performing that refactoring.

Cheers,
cu, WMD.


On Wed, Sep 18, 2013 at 3:08 AM, Joel Borggren-Franck
<joel.franck at oracle.com> wrote:
> Hi Werner,
>
> In general I think this looks good.
>
> On 2013-09-15, Werner Dietl wrote:
>> Jon, all,
>>
>> as you suggested, I encapsulated the typeAnnotations and underlyingType
>> fields in com.sun.tools.javac.code.Type.AnnotatedType
>>
>> This actually made the code simpler in a few places.
>
>> There is one new failure:
>> langtools/test/tools/javac/processing/model/type/BasicAnnoTests.java
>>
>> However, I think the test case is wrong: it uses a "normal" annotation
>> processor and therefore runs too early. It should be changed to be a
>> "type annotation processor", i.e. it should run after FLOW.
>> I suggest we create a subclass/alternative to
>> JavacTestingAbstractProcessor and use that to test type annotations.
>
> Annotation processing will occur before flow. Eventually we need to move
> to a model were we trigger stuff that annotation processing depends on
> before attr. Some type anno stuff needs to be done there, and some can
> be delayed. For example there is this bug:
> https://bugs.openjdk.java.net/browse/JDK-8014016 that is assigned to Jan
> in the javac team.
>
> I'm not sure the concept of an after flow processor makes sense at all.
>
> However it seems a bit strange that your encapsulation introduced an
> error like this.
>
>>
>> I'm not sure whether this had an effect on javadoc - if so, we should
>> revisit when the typeAnnotations queue is flushed.
>>
>> Please review this changeset:
>>
>> http://hg.openjdk.java.net/type-annotations/type-annotations/langtools/rev/6f595e036db7
>>
>> If this first step is OK, we can next introduce the TypeMetadata
>> abstraction.
>>
>>
>> Regarding SymbolMetadata, I find it a bit strange that the methods that
>> access SymbolMetadata in Symbol are still called "xxxAnnotations", e.g.
>> com.sun.tools.javac.code.Symbol.getAnnotations()
>> Should these also be renamed?
>>
>
> I'm working towards removing those altogether (but probably in the 9
> timeframe). There was I reason I didn't want Andreas to rename them, but
> I can't recall it now :)
>
>> I have one more change that I just pushed:
>>
>> http://hg.openjdk.java.net/type-annotations/type-annotations/langtools/rev/e1772bcdf768
>>
>> I changed com.sun.tools.javac.tree.TreeCopier.visitLabeledStatement
>> to take the copied body instead of the original body.
>> Charlie Garrett discovered this problem after painful debugging of a
>> Checker Framework problem that depended on whether a label is present or
>> not.
>> I cannot present a simple test case for the error, but the change
>> doesn't break any existing test cases.
>> Also note that this was the only "unused variable" warning in
>> TreeCopier. Warnings are useful.
>>
>
> Seems reasonable.
>
> cheers
> /Joel



-- 
http://www.google.com/profiles/wdietl