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
