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:

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 

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):


Is it easier on the eye now?

I also added Copyright headers to all the test sources.

Regards, Peter

Reply via email to