RFR: JDK-8210031: implementation for JVM Constants API

[Roger Riggs]

Hi Vicente,

On 12/05/2018 09:13 PM, Vicente Romero wrote:
> 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/
>
>
>>
>> - 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
My point is that the javadoc is a specification; so the reader should 
not need to
look at an implementation to completely understand the required behavior.
>
>>
>> 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?
In other places in java specs, there is a distinction made between 
'inner' classes and 'nested' classes.
If the same API is used for both, it would be clearer to the reader if 
it mentioned nested.

Thanks, Roger
>
>>
>> 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
>>>
>>
>