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()
.current("b.PublicSub").member("a.PublicSuper").target("b.PublicSub")
.allowed(PROTECTED_INSTANCE_F_M, PUBLIC_INSTANCE_F_M,
PROTECTED_STATIC_F_M,
PUBLIC_STATIC_F_M, PUBLIC_C)
.perform();
...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()
.current("b.PublicSub").member("a.PublicSuper").target("b.PublicSub")
.allowed(PROTECTED_INSTANCE_F_M, PUBLIC_INSTANCE_F_M,
PROTECTED_STATIC_F_M,
PUBLIC_STATIC_F_M, PUBLIC_C)
.denied (PRIVATE_INSTANCE_F_M, PACKAGE_INSTANCE_F_M,
PRIVATE_STATIC_F_M,
PACKAGE_STATIC_F_M, PRIVATE_C, PACKAGE_C,
PROTECTED_C)
.perform();
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
tests):
http://cr.openjdk.java.net/~plevart/jdk9-dev/6378384_Reflection.ensureAccess/webrev.03/
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.
thanks
Mandy
Regards, Peter
