RFR: JDK-8210031: implementation for JVM Constants API

[Vicente Romero]

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