Public review for 8040327: Eliminate AnnotatedType

[Werner Dietl]

Hi Eric,

I had a quick look and notice:

com/sun/tools/javac/code/Printer.java:

+        List<Attribute.TypeCompound> annos = t.getAnnotationMirrors();
+        if (!annos.isEmpty()) {
+            if (prefix) sb.append(' ');
+            sb.append(t.getAnnotationMirrors());

The last line could use annos instead of calling t.getAnnotationMirrors again.


+    private void printBrackets(Type t, StringBuilder sb, Locale locale) {
         Type arrel = t;
         while (arrel.hasTag(TypeTag.ARRAY)) {
-            if (arrel.isAnnotated()) {
-                sb.append(' ');
-                sb.append(arrel.getAnnotationMirrors());
-                sb.append(' ');
-            }
+            sb.append(printAnnotations(t, true));
             sb.append("[]");
-            arrel = arrel.unannotatedType();
             arrel = ((ArrayType) arrel).elemtype;
         }
     }

This change seems wrong. Previously, the type annotations on arrel
were printed on each iteration of the loop.
Now, each iteration prints the annotations on "t" - the top-level
type. The added line should be
sb.append(printAnnotation(arrel, true));


     @Override
     public String visitMethodType(MethodType t, Locale locale) {
-        return "(" + printMethodArgs(t.argtypes, false, locale) + ")"
+ visit(t.restype, locale);
+        return printAnnotations(t) + "(" +
+            printMethodArgs(t.argtypes, false, locale) + ")" +
+            visit(t.restype, locale);
     }

What is the meaning of type annotations on a MethodType? We have TAs
on the return type of a method or constructor, but those are in
t.restype.
I don't see what annotations a MethodType itself should contain and
why they would be separate from restype.
Should TAs on the receiver of a method be printed instead (in the
right location)?

Same comment for MethodType.toString() later in the diff.
MethodType.annotatedType correctly raises an error, so I don't see why
toString outputs it.


I'll wait with adapting the Checker Framework until all three patches
are available and can then give more detailed feedback.

cu, WMD.


On Wed, May 7, 2014 at 7:08 PM, Eric McCorkle <eric.mccorkle at oracle.com> wrote:
> Hello,
>
> This is the first of a series of patches which implement a significant
> overhaul of type annotations code in the javac frontend.  This patch
> eliminates the AnnotatedType class, replacing its functionality by
> storing annotations on the Type class itself.
>
> This will eventually be rolled into a more general type metadata
> framework that has been planned as part of ongoing work.
>
> Note that this patch is also being reviewed by the langtools team using
> the Crucible tool.
>
> The webrev can be found here:
> http://cr.openjdk.java.net/~emc/8040327/
>
>
> The following patches are also under initial review, and will be posted
> soon:
>
> 1) A patch which replaces much of the functionality in
> TypeAnnotations.java with code integrated directly into the MemberEnter
> and Attr phases.  The end result is that Attribute.TypeCompound objects
> will now contain the correct position from the moment they are created,
> and the position field will be final.  This patch is known colloquially
> as the "positions patch", though its scope has grown considerably beyond
> that.
>
> 2) A follow-up patch which removes code dead code that was replaced by
> the positions patch, and removes some code duplication that was
> introduced by that patch.



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