RFR: JDK-8210031: implementation for JVM Constants API

[Vicente Romero]

Hi Roger,

Thanks for your comments, see my comments below. I have published 
another iteration at [1]

Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8210031/webrev.12/

On 12/5/18 4:35 PM, Roger Riggs wrote:
> Hi,
>
> Editorial comments java.lang.contant package javadoc:
>
>  - In the Nominal Descriptors  2nd paragraph:  "of of" -> "of"
>
> -  DirectMethodHandleDescImpl - Its unusual to see a Class named *Impl in
>    a developer facing API.  And if its intended, it should be linked.

DirectMethodHandleDescImpl is not public

>
> - "These classes provides" plural agreement.
>
>
> DynamicCallSiteDesc:
>
>  - Is the format of names clearly specified:  IAE -> if the invocation 
> name has the 'incorrect format'.

I think so, depending on the case it the related part of the JVMS is 
added as a reference

>
>  - typo:   "descripbing" - > "describing"
>
>  - missing @throws IAE for method :
>
>    ​of(DirectMethodHandleDesc
> <http://cr.openjdk.java.net/%7Evromero/8210031/javadoc.18/java/lang/constant/DirectMethodHandleDesc.html> 
>  bootstrapMethod,String
> <http://cr.openjdk.java.net/%7Evromero/8210031/javadoc.18/java/lang/String.html> 
>  invocationName,MethodTypeDesc
> <http://cr.openjdk.java.net/%7Evromero/8210031/javadoc.18/java/lang/constant/MethodTypeDesc.html> 
>  invocationType)
>
>
> - The withArgs method only throws NPE:  is there nothing else that can 
> go wrong?
the rest of the arguments passed to the invocation to constructor 
DynamicCallSiteDesc has already been checked so no other exceptions 
associated to them should be thrown
>
> - bootStrapArgs() - the return is always non-null, with zero length 
> array for non args?

it has to be, IMO, as the private constructor that is the only one that 
assigns a value to field bootstrapArgs makes sure that the assigned 
value is non-null

>
> DynamicConstantDesc.java:
>  - First sentences should end with period. "."
>
>  - ofCanonical:  2nd sentence, "Classes ... produces ... a 
> ConstantDesc"; grammar?
>
>  - of Canaonical(): So a conforming implementation is not required to 
> return the
>     well known values, only suggested.
>
> ClassDesc.java:
>  - The create methods should not be required to create new instances.  
> Use "return"  instead of "create".
>
>  - How are class descriptors created for 'nested' classes (as opposed 
> to inner classes).
>    It is worth describing or referencing how that should be done.

not sure I get what you mean. If you want to create a class descriptor 
for a nested class you should be able to invoke inner() with an string 
containing a number, what should be made more clear here in your opinion?

>
>  - Can inner() throw IAE for invalid format names?
yes added
>
>  - How can the rank of an array ClassDesc be found?

we can add a method for this after the integration

>
> Constable.java:
>
>  - Avoid ending the first sentence of describeConstable description 
> with "be".
>
> ConstDesc.java:
>  - "with respect to a particular to a class loader" ; extra words "to a"?
>
> DirectMethodHandleDesc.java:
>  - ofField, @throws IAE might mention the possibility of an invalid name

right
>
> MethodTypeDesc.java:
>  - First sentences should end with period. "."
>
> Regards, Roger
>
>
> On 12/05/2018 01: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
>>>
>>
>> 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?
>>
>> 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
>>
>