Hi Mandy,
Thanks for looking into this patch.
On 10/15/2016 11:31 PM, Mandy Chung wrote:
Hi Peter,
On Oct 2, 2016, at 2:51 PM, Peter Levart <peter.lev...@gmail.com> wrote:
http://cr.openjdk.java.net/~plevart/jdk9-dev/6378384_Reflection.ensureAccess/webrev.02/
This change looks good to me. Thanks for adding the test enumerating
exhaustive combinations. I ran JCK tests and verified that no new failure.
With this change, sun.reflect.misc.ReflectUtil.ensureMemberAccess could be
removed when Atomic*FieldUpdater are updated to use
Reflection::ensureMemberAccess directly. As of now, we will keep
ReflectUtil::ensureMemberAccess.
AccessControlTest.java is a great test. I wonder if AccessControlTest.java
could be made less verbose and make it easier to read. For example, can we add
a builder class that takes either the list of allowed or denied MemberFactory
(but not both) to reduce the verbosity.
I think specifying both is more verbose on one hand, but OTOH it allows
one to visually inspect all the cases and think of each and every one in
isolation to see if it is valid in a particular caller/member/target
arrangement. The test infrastructure verifies that the test case covers
all the MemberFactory(s) so one must only verify each individual allowed
/ denied MemberFactory.
Builder::allowAll and Builder::denyAll would be useful. allowAccessMember
of a specific modifier can imply both field and method.
This is a good idea. Here's a modified test (no changes to patched
files, just tests):
http://cr.openjdk.java.net/~plevart/jdk9-dev/6378384_Reflection.ensureAccess/webrev.03/
Is it easier on the eye now?
I also added Copyright headers to all the test sources.
Regards, Peter
