RFR: JDK-8210031: implementation for JVM Constants API
Vicente Romero
vicente.romero at oracle.com
Thu Dec 6 18:37:34 UTC 2018
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
>>>>
>>>
>>
>
More information about the core-libs-dev
mailing list