[Nestmates] RFR: 8197539: [Nestmates] Revert all changes to VerifyAccess.isSameMemberPackage and Lookup.in behaviour
david.holmes at oracle.com
Thu Feb 15 22:54:48 UTC 2018
On 16/02/2018 8:12 AM, Karen Kinnear wrote:
> Code looks good. Many thanks David.
> I think this particular comment would be confusing, so I would leave it alone.
> This captures the legacy case and continues backward compatibility for inner/outer classes that
> also happen to be nestmates.
> It is not intended to work for all nestmates - the general case is that MethodHandle/VarHandle
> behavior will match the bytecode behavior. That is covered by the changes to allow access
> to private members for nestmates and should not use this old workaround.
>> On Feb 13, 2018, at 4:50 PM, David Holmes <david.holmes at oracle.com> wrote:
>> Hi Mandy,
>> Thanks for looking at this.
>> On 14/02/2018 7:10 AM, mandy chung wrote:
>>> On 2/11/18 7:32 PM, David Holmes wrote:
>>>> webrev: http://cr.openjdk.java.net/~dholmes/8197539/webrev/
>>>> webrev relative to mainline: http://cr.openjdk.java.net/~dholmes/8197539/webrev.mainline/
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8197539
>>>> After going through the issue in JDK-8197395, there is no need to change VerifyAccess.isSameMemberPackage to be aware of nestmates, as nestmates will already pass the enclosing class check. So we can restore this code to its existing mainline form.
>>> That's right. It'd be good to add a comment to indicate that this captures both legacy and nestmate case.
>>> 359 if (getOutermostEnclosingClass(class1) != getOutermostEnclosingClass(class2))
>> I would prefer to leave unmodified code completely unmodified to simplify the eventual code review when pushing to mainline. While such a comment may seem useful due to the way this issue arose, if we back up and view this as never-changed code, then commenting that it doesn't need to change seems unnecessary to me. There are numerous things that work unchanged with nestmates (specifically anything to do with inner classes attributes and existing enclosing class notions), and we won't want to add comments to them all.
More information about the valhalla-dev