[Nestmates] RFR: 8196320: [Nestmates] Restore the old enclosing-class based isSamePackageMember check in sun/invoke/util/VerifyAccess.java

[David Holmes]

Thanks Mandy!

David

On 2/02/2018 5:34 AM, mandy chung wrote:
> 
> 
> On 1/31/18 10:47 PM, David Holmes wrote:
>>
>>> OK (I was confused with the semantics of JVM_AreNestMates).  It does 
>>> return true on the same class.   It would help if you can add the 
>>> comment describing this function in jvm.h.
>>
>> Most methods in jvm.h are not documented. The assumption seems to be 
>> that the semantics are defined as per the JDK method that calls them - 
>> in this case Reflection.areNestMates. In this case, like many 
>> Reflection methods, that wasn't documented either (oops!) so I've 
>> added some basic docs:
>>
>>  /**
>>    * Returns true if {@code currentClass} and {@code memberClass}
>>    * are nestmates - that is, if they have the same nesthost as
>>    * determined by the VM.
>>    */
>>
> 
> Thanks.
>>>>
>>>> Callers to Reflection.areNestmates (which calls JVM_AreNestMates) 
>>>> can do their own "optimizations" if they wish to pre-check if we are 
>>>> dealing with the same class or different packages - as the original 
>>>> isSameMemberPackage check does.
>>>
>>> These optimization in VerifyAccess::areNestMates can be moved to 
>>> Reflection::areNestMates so that it's clearer the difference is the 
>>> top-level enclosing class check.
>>
>> Reflection.areNestMates is a native method. Any fast-paths belong in 
>> the callers ie VerifyAccess.areNestMates, or 
>> Reflection.verifyMemberAccess - and indeed they do exist there.
>>
>> If you are concerned about confusion between VerifyAccess.areNestMates 
>> and Reflection.areNestMates I can return to using 
>> VerifyAccess.isSamePackageMember - though I find that a very poor name 
>> to begin with and keep writing it the wrong way round. 
>> (isInSamePackageMember would be more accurate.)
> 
> It's okay to leave it as is.   Two same method names doing different 
> check does cause some confusion but I think the real confusion to me is 
> that core reflection does not match the language access check in this case.
> 
>>
>> I've reverted to -source/-target and hope that will not induce an API 
>> check (though will probably produce a warning). If it does do the API 
>> check then I'll have to factor the test classes into a separate file 
>> as you say.
>>
>> Updated webrev: http://cr.openjdk.java.net/~dholmes/8196320/webrev.v2/
>>
> Looks okay to me,
> 
> Mandy