Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Chris Plummer]

On 4/18/20 3:28 PM, Mandy Chung wrote:
> Hi Chris, Serguei,
>
> On 4/18/20 12:18 AM, Chris Plummer wrote:
>>
>> Yes. I'd like to see all this as part of the Class/Classloading spec 
>> in some sort of section that gives an overview of all these topics, 
>> but mostly clarifies what an initiating loader is, and the (non) 
>> relationship to hidden classes. 
>
> We should refer initiating loader and class loading from JVMS 5.3.   
> JVM TI needs the clarification w.r.t.  GetClassLoaderClasses that does 
> not include hidden classes because the initiating class loader cannot 
> find it.
>
> GetLoadedClasses is about class creation.   While it seems tempted to 
> put the descriptions in some sort of overview section, I found the 
> clarification is specific for each function and hence inlining them in 
> each function helps the readers directly see the difference.
>
> I made a few changes that should ease your concern of duplication:
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes.01/
>
> - The class creation description added in GetLoadedClasses.  Not in 
> JDWP, JDI and Instrumentation.
> - The description in GetClassLoaderClasses, 
> Instrumentation::getInitiatedClasses, 
> ClassLoaderReference::visibleClasses is revised to take out the 
> details.   Add links from JDWP and JDI to GetClassLoaderClasses.
> - The details about hidden classes can be found from `Class::isHidden` 
> and      `Lookup::defineHiddenClass`.
>
> Mandy
Hi Mandy,

This looks much better. Thank you for the consolidating and the cross 
references. I think that's a big improvement.

One minor thing I noticed is a few "JNI-style signature" references. In 
LocalVariable.java you renamed "JNI signature" to "JNI-style signature", 
but in JDI Mirror.java you renamed "JNI-style signature" to "type 
signature". And then in JDI TypeComponent.java you left the "JNI-style 
signature" reference unchanged. Why the inconsistency?

In VirtualMachine.java:

  145      * @see Class or interface creation specified in
  146      * <a 
href="{@docRoot}/../specs/jvmti/jvmti.html#GetLoadedClasses">JVM TI 
GetLoadedClasses</a>

How about:

  145      * @a description of how Class and interface creation can be 
triggered is specified in
  146      * <a 
href="{@docRoot}/../specs/jvmti/jvmti.html#GetLoadedClasses">JVM TI 
GetLoadedClasses</a>

I read the following incorrectly a few times before I finally understood 
the meaning:

   44     /**
   45      * Returns the {@linkplain com.sun.jdi.Type#name() name of the 
class}
   46      * that has been unloaded. The returned string may not be a
   47      * <a 
href="${docRoot}/java.base/java/lang/ClassLoader.html#binary-name">binary 
name</a>.
   48      *
   49      * @see Class#getName()
   50      */
   51     public String className();
   52
   53     /**
   54      * Returns the {@linkplain com.sun.jdi.Type#signature() type 
signature of the class}
   55      * that has been unloaded.  The returned string may not be a 
type descriptor
   56      * specified in JVMS {@jvms 4.3.2}.
   57      *
   58      * @see java.lang.invoke.TypeDescriptor#descriptorString()
   59      */
   60     public String classSignature();

"may not be" through me off. I read it as "is not allowed not be". Your 
intent was "might not be", which might be a better choice of words.

thanks,

Chris