Initial proof of concept for implementation of -Xlint:hiddenclass (original) (raw)

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Fri Sep 21 06:14:05 PDT 2012

On 20/09/12 13:31, Maurizio Cimadamore wrote:

Excellent - I would add a test to check that the flag work under separate compilation environment. Sorry - your test already stress the separate compilation case; you need a test in which the auxiliary class is accessed from within the source in which it was declared (which would stress the code in Enter).

Also, I would move the flag logic from Enter to MemberEnter.complete - which is the place where to set symbol flags directly on Symbol.flags_field.

Maurizio

Maurizio On 20/09/12 13:23, Fredrik Öhrström wrote: Third webrev: http://cr.openjdk.java.net/~ohrstrom/webrev-7153951-v3

2012/9/13 Jonathan Gibbons <jonathan.gibbons at oracle.com>: The arity of the message is wrong. I Check 3111, you provide 3 args. In the resource file you only expect 2.

-- Jon

On 09/12/2012 07:07 PM, Jonathan Gibbons wrote: In the message file, the comment on 1721 is redundant since the same info is on line 1722. These comments are used by the localization team. They are in a stylized form so that they can be mechanically checked by utilities in the test/tools/diags/examples world. Since there is a new message, you will need a new example, in order to pass the tests. Check:3107 The convention is to use the kind, not use instanceof -- Jon On 09/13/2012 08:06 AM, Maurizio Cimadamore wrote: On 12/09/12 16:00, Fredrik Öhrström wrote: Second round of implementation: http://cr.openjdk.java.net/~ohrstrom/webrev-7153951-v2/ <http://cr.openjdk.java.net/%7Eohrstrom/webrev-7153951-v2/>

When building the openjdk, it outputs 316 warnings like these: .../ClientHandshaker.java:723: warning: secondary class SupportedEllipticCurvesExtension in …./HelloExtensions.java should not be accessed from outside its own source file Great work - couple of comments: *) it seems like the warning should not be generated when isAccessible fails, so, why not using that method as a condition to filter out the warning (instead of inlining that logic in the method guard) ? *) is there a better name than 'secondary class' (maybe a question for our spec gurus?) Maurizio

//Fredrik 16 mar 2012 kl. 16:03 skrev Jonathan Gibbons: Fredrik, Good start. 1. It should be sufficient to remove references to the file manager and use something like c.sourcefile == env.toplevel.sourcefile to check if the class is in the "right" file. 2. I think line numbers are desirable, and would be easy if you move the checkForHiddenAccess from Resolve into Check, and call it from Attr. 3. I think a warning per reference is acceptable: it is not necessary to optimize it to a warning per hidden class. 4. We'll need a CCC approval for the new Xlint suboption. 5. We should check to see if there is a preferred term for what you call "hidden class". -- Jon

On 03/16/2012 05:12 AM, Fredrik Öhrström wrote: From the bug: Although legal, the use of multiple top level classes in the same file is somewhat questionable to begin with, but it is particularly bad when in some package class A in A.java refers to class B defined in C.java. This requires that at times the files must be compiled together, and prevents implicit compilation from locating such "hidden classes". Proof of concept impl: http://cr.openjdk.java.net/~ohrstrom/webrev-7153951-v1/ <http://cr.openjdk.java.net/%7Eohrstrom/webrev-7153951-v1/> It seems to detect the problem correctly. And there are a few of these in the jdk, it seems, ~100. How to do isSameFile test properly? How to report each hidden class reference only once? Are line numbers neccesary? Where to put checkForHiddenAccess? Does it cover all possible use cases? What else did I miss? //Fredrik

More information about the compiler-dev mailing list