RFR: JDK-8210031: implementation for JVM Constants API
Roger Riggs
Roger.Riggs at oracle.com
Wed Dec 5 21:35:39 UTC 2018
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.
- "These classes provides" plural agreement.
DynamicCallSiteDesc:
- Is the format of names clearly specified: IAE -> if the invocation
name has the 'incorrect format'.
- 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?
- bootStrapArgs() - the return is always non-null, with zero length
array for non args?
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.
- Can inner() throw IAE for invalid format names?
- How can the rank of an array ClassDesc be found?
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
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