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

[Chris Plummer]

On 4/20/20 1:57 PM, Mandy Chung wrote:
>
>
> On 4/20/20 1:06 PM, Chris Plummer wrote:
>> 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". 
>
> You meant Type::signature.   Type::signature may return the type 
> signature of a hidden class whereas LocalVariable::signature and 
> TypeComponent::signature return the field/method descriptor where no 
> hidden class will appear in a resolved field/variable/method.
ok
>
>> And then in JDI TypeComponent.java you left the "JNI-style signature" 
>> reference unchanged. Why the inconsistency?
>>
> I made LocalVariable::signature consistent with 
> TypeComponent::signature whereas Type::signature returns the string of 
> the form described in Class::descriptorString.
>
> I tried to keep existing reference to use "JNI-style signature". Since 
> I touched on these files, I can change them to "type signature" which 
> applies to these cases but I will leave the link to the JNI spec as it is.
ok
>
>> 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>
>>
>
>      * @see <a 
> href="{@docRoot}/../specs/jvmti/jvmti.html#GetLoadedClasses">
>      * JVM TI GetLoadedClasses</a> regarding how class and interface 
> creation can be triggered
That's better.
>
>> 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.
>>
>
> I make it clearer as:
>
>      * Returns the {@linkplain com.sun.jdi.Type#signature() type 
> signature of the class}
>      * that has been unloaded.  The result is of the same
>      * form as the string returned by {@link Class#descriptorString()}.
>      * If this class can be described nominally, the returned string is a
>      * type descriptor conforming to JVMS {@jvms 4.3.2}; otherwise, 
> the returned string
>      * is not a type descriptor.
>
> Similar clarification is also made in Type::signature:
>
>      * Returns the type signature for this type.  The result is of the 
> same
>      * form as the string returned by {@link Class#descriptorString()}.
>      * The returned string is a type descriptor conforming to JVMS 
> {@jvms 4.3.2}
>      * if this type can be described nominally.  Otherwise, the 
> returned string
>      * is not a type descriptor.
Great!

thanks,

Chris
>
> Mandy