Hi Mandy,
On 12/5/18 1:12 PM, Mandy Chung wrote:
On 12/3/18 11:12 AM, Vicente Romero wrote:
Hi all,
Can I have the final nod to the JVM constants API, there have been
some changes since the last review iteration. Thanks to the internal
and external developers that have taken the time to provide feedback
so far. The links to the last versions are:
webrev: http://cr.openjdk.java.net/~vromero/8210031/webrev.10/
javadoc:
http://cr.openjdk.java.net/~vromero/8210031/javadoc.18/overview-summary.html
specdiff:
http://cr.openjdk.java.net/~vromero/8210031/specdiff.08/overview-summary.html
Thanks for your suggestions I have fixed them.
I reviewed webrev.10 and overall looks good to me. A couple of minor
comments and some of them seems to be fixed already in amber repo. No
need to generate a new webrev.
Nit: The javadoc of the new methods starts with "Returns", "Return",
"Produce", "Create", "Resolve", "Compares" etc. It'd be good to do a
pass and fix the verb form consistently.
src/java.base/share/classes/java/lang/Class.java
nit: Line 163 seems to have extra white-spaces, not aligned with
the other superinterfaces.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassWriterImpl.java
74 private static final Set<String> suppressSubtypesSet
75 = Set.of("java.lang.Object",
76 "org.omg.CORBA.Object");
This is not related to your change. CORBA is no longer in JDK.
Maybe this special casing is no longer needed. It may worth
filing a JBS issue to examine this.
ConstantDescs.java
64 // Don't change the order of these declarations!
Is there any code depending on this order?
not really the order is in increasing `complexity` but there is no hard
dependency on it just to keep a logical order
DirectMethodHandleDesc.java
46 * {@link MethodHandle}. A {@linkplain DirectMethodHandleDescImpl}
corresponds to
typo: DirectMethodHandleDescImpl
57 /**
58 * Kinds of method handles that can be described with {@linkplain
DirectMethodHandleDesc}.
59 */
60 enum Kind {
This needs @since 12
DynamicCallSiteDesc.java
59 * @param bootstrapMethod a {@link DirectMethodHandleDescImpl}
describing the
60 * bootstrap method for the {@code
invokedynamic}
typo: DirectMethodHandleDescImpl and in a few other @param
Mandy
Thanks,
Vicente
