Hi Mandy,

On 10/17/2016 10:52 PM, Mandy Chung wrote:
On Oct 16, 2016, at 11:18 AM, Peter Levart <peter.lev...@gmail.com> wrote:

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.
I understand the intention there.  But when each test case enumerates 20 
constants, it’s getting harder to review what’s going on and catch any issue.

It now enumerates only 12 constants and I think it's quite the opposite. Take the following for example. If only the allowed members were listed:

         ok &= new Test()
                      PUBLIC_STATIC_F_M, PUBLIC_C)

...you would review the members that are allowed and then you should ask yourself: "Which are the ones that are denied? All the rest. What are they?", or: "Could there be any other that should be allowed? Which one?"

It's much easier if they are explicitly listed:

         ok &= new Test()
                      PUBLIC_STATIC_F_M, PUBLIC_C)
                      PACKAGE_STATIC_F_M, PRIVATE_C, PACKAGE_C,

And besides, you don't really have to review them all. The fact that running the test on unpatched JDK 9 finds just two differences:

- access to protected static method from subclass in another package
- access to protected static field from subclass in another package

...is a reassurance that the patch does exactly what it should. No less, no more.

    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 

I like these grouping and it does help.  One more nit: would be good to replace 
the class name strings with constant variables.  I don’t need a new webrev.

Then I would have to have a mapping from class name -> constant name for the generator. Besides, constant names would not be any prettier than class name string literals. At least now it is obvious to anyone what package a particular class belongs to:

    "a.Package" vs. A_PACKAGE ?

Note that I can't use a.Package.class literal(s) here (thought I would like to) as they don't compile if they refer to a package-private class from another package.

I would like to keep those things unchanged, If you don't mind.


Regards, Peter

Reply via email to