[core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[David Holmes]

Hi Mandy,

On 22/05/2018 1:23 PM, mandy chung wrote:
> Hi David,
> 
> I reviewed:
> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
> 
> Looks good in general.  I have a couple other comments.  I will review 
> the new version that includes new tests in test/jdk/java/lang/reflect 
> when it's ready.

Thanks! That should be out later today.

> Class.java
> 
> 3988 Class<?>[] members = getNestMembers0();
> 
> If the above fails with any LinkageError, what is the expected behavior 
> if Class::getNestMembers() is called the second time?  will it throw the 
> same LinkageError?

Based on the implementation it should throw the same error. We use 
constant-pool resolution to locate the member classes so if resolution 
fails then it must continue to fail for the same reason per the JVMS rules.

Are you thinking that getNestMembers() should say something about this?

> On 5/20/18 10:57 PM, David Holmes wrote:
>>
>>> I think it's okay in VerifyAccess too but the "FIX ME" suggests there 
>>> are doubts so I assume this needs more eyes to triple check.
>>
>> The "FIX ME" is not about the access check but the form of the 
>> assertion. The assert is verification that resolution of a private 
>> method always leads to selection of that private method: refc == defc. 
>> I used "myassert" so that this check was always enabled during 
>> testing. The "FIX ME" was to either convert to language assert 
>> statement or else remove it (having validated that it never fires). 
>> Obviously "myassert" has not fired in all the testing that I have done 
>> so either choice seems fine. Do you have a preference? 
> 
> Since you have the assert, I would convert it to language assert that 
> would help the readers to understand the result of such resolution.

Will do.

> Regarding Alan's and Peter's comment on the spec clarification on 
> primitive type and array type, it is reasonable to take a pass on all 
> reflection APIs and make the text explicit consistently if this class is 
> a primitive type and array type.   I will file an issue to track this.   
> Class::getModifiers does return the modifier of the component type 
> whereas Class::getEnclosingClass returns null if this class is an array 
> type even if the component type has an enclosing class.   Clarification 
> would help developers.

Thanks. I will add the small clarification that Alan requested on 
getNestHost().

Thanks,
David

> Mandy