RFR: JDK-8210031: implementation for JVM Constants API

[Vicente Romero]

Hi Rogers,

Thanks for your comments, I have uploaded another webrev [1], some 
comments below:

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

On 12/6/18 11:07 AM, Roger Riggs wrote:
> 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.

got it, I added a comment to the specification to make it more clear

>>
>>>
>>> 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.

the ::inner methods were renamed to ::nested

>
> Thanks, Roger

Thanks
Vicente

>>
>>>
>>> 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
>>>>
>>>
>>
>