Please review the change to access the annotations from module-info
Mandy Chung
mandy.chung at oracle.com
Wed Apr 28 16:26:45 PDT 2010
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
More information about the jigsaw-dev
mailing list