Please review the change to access the annotations from module-info

[Mandy Chung]

Joe,

Thanks for the review.

Joe Darcy wrote:
> Hello.
>
> Some comments from a first pass over the code:
>
> In ModuleInfo.java, I recommend adding an "@see 
> java.lang.reflect.AnnotatedElement#isAnnotationPresent" to the 
> isAnnotationPresent method.
>

Added.

> In ModuleInfoReader.java, if you want to start using some new language 
> features, the following line is an ideal candidate for the diamond 
> operator:
>

Thanks for the new language features.  Will switch to use that.

> 70     private Map<String, ModuleInfoAnnotation> annotationTypes = new 
> LinkedHashMap<String, ModuleInfoAnnotation>();
>
> With diamond
>
> 70     private Map<String, ModuleInfoAnnotation> annotationTypes = new 
> LinkedHashMap<>();
>
> Later in that file, this looks like a candidate for strings in switch:
>
> 135             if (name.equals(MODULE))
> 136                 readModule();
> 137             else if (name.equals(MODULE_PROVIDES))
> 138                 readModuleProvides();
> 139             else if (name.equals(MODULE_REQUIRES))
> 140                 readModuleRequires();
> 141             else if (name.equals(MODULE_PERMITS))
> 142                 readModulePermits();
> 143             else if (name.equals(MODULE_CLASS))
> 144                 readModuleClass();
> 145             else if (name.equals(RUNTIME_VISABLE_ANNOTATION)) {
> 146                 readAnnotations();
> 147             } else {
> 148                 in.skip(length);
>
> switch(name) {
>    case MODULE:
>        readModule(); break;
>
>    case MODULE_PROVIDES:
>    ...
> }
>
> In src/share/classes/java/lang/module/ModuleSystem.java, this method body
>
>  43     // Module names must be legal Java identifiers
>  44     //
>  45     public static final String checkModuleName(String nm) {
>  46         if (nm == null)
>  47             throw new IllegalArgumentException();
>  48         int n = nm.length();
>  49         if (n == 0 || !Character.isJavaIdentifierStart(nm.charAt(0)))
>  50             throw new IllegalArgumentException();
>  51         for (int i = 1; i < n; i++) {
>  52             char c = nm.charAt(i);
>  53             if (!Character.isJavaIdentifierPart(c) && (c != '.'))
>  54                 throw new IllegalArgumentException(nm
>  55                                                    + ": Illegal 
> module-name"
>  56                                                    + " character"
>  57                                                    + " at index " 
> + i);
>  58         }
>  59         return nm;
>  60     }
>
> could be replaced by a call to SourceVersion.isIdentifier:
> http://download.java.net/jdk7/docs/api/javax/lang/model/SourceVersion.html#isIdentifier(java.lang.CharSequence) 
>
>
> An advantage of the isIdentifier method is that it does the right 
> thing in regard to code points; see the code in
> http://hg.openjdk.java.net/jdk7/jdk7/langtools/file/71c2c23a7c35/src/share/classes/javax/lang/model/SourceVersion.java 
>
> for the codepoint-correct loop if you can't call this method.
>
Thanks for the pointer.   I'll replace it with the codepoint correct 
loop.  javax.lang.model is not in the boot module and so we can't call it.

> /src/share/classes/java/lang/reflect/Module.java
>
> Is there a reason the annotation handling code here looks different 
> from the analogous code in java.lang.reflect.Constructor?
>

module-info.class is not loaded as a Class.  Instead, it's parsed by 
ModuleInfoReader.  So we can't use AnnotationParser.parseAnnotations 
unless we create a ClassLoader to load module-info.class into the VM.

> Should Module extend AccessbileObject?
>

Good question.  I don't think so.  Alex and Mark, what do you think?

> src/share/classes/java/lang/module/ModuleInfoAnnotation.java
>
>  53  * Represents an annotation in module-info.class. This class is
>  54  * based on the com.sun.tools.classfile library.
>  55  *
>  56  * @see com.sun.tools.classfile.Annotation
>
> I don't think the official javadoc of a java.lang.* class should 
> reference a com.sun.* class.

I can take that out.  The ModuleInfoAnnotation is a package-private 
class and so I would think it's okay.
>
> As to how this class is structured, I'm not familiar with the origin 
> of provenance of com.sun.tools.classfile.Annotation.  However, perhaps 
> the time has come to have a unified "arms length annotation maker" to 
> subsume the functionality in 
> langtools/src/share/classes/com/sun/tools/apt/mirror/declaration/AnnotationProxyMaker.java 
> and a similar class in javac.
>
> src/share/classes/java/lang/module/UnsupportedElementTypeException.java
>
>  44  */
>  45 public class UnsupportedElementTypeException extends 
> RuntimeException {
>  46  47     private static final long serialVersionUID = 269;
>  48  49     private transient List<String> classnames; // cannot be 
> serialized
>
> I assume you used javax.lang.model.type.MirroredTypeException[s] for 
> inspiration here :-)  

I copied from MirroredTypeExceptions but forget to clean it up :)
> A difference is that the classname UnsupportedElementTypeException 
> should be able to be serialized since it is just a list of strings; 
> the MirroredTypeExceptions cannot be serialized because the Type 
> objects are tied up in a lot of internal javac state.  (And if 
> UnsupportedElementTypeException is made fully serializable, it should 
> define a good serial form for the classname.)
>
Will clean that up.

> As a general comment, I would encourage shell regression tests to be 
> rewritten as java tests.
>

It was good to know that langtools shell regression tests were converted 
to java tests.  I will do that some time.

Mandy