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