RFR: JDK-8210031: implementation for JVM Constants API

[Roger Riggs]

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
>