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.
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.
Thanks, Roger
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
