Code review request: 7146763: Warnings cleanup in the sun.rmi and related packages

Fri Mar 2 02:43:50 UTC 2012

Looks good! Thanks for making the updates.

Interestingly, sun/rmi/server/LoaderHandler.java has a couple methods that use 
Class[] as a parameter (loadProxyClass, loadProxyInterfaces), but which don't 
generate rawtypes warnings. I may investigate this at some point.

Anyway, I think this has gone through enough rounds of review. Go ahead and 
push. Thanks again.

s'marks

On 3/1/12 9:48 AM, Kurchi Hazra wrote:
> Hi Stuart,
>
> Please find an updated webrev here:
> http://cr.openjdk.java.net/~khazra/7146763/webrev.05/
>
>
> Thanks,
> Kurchi
>
>
> On 2/28/2012 4:32 PM, Stuart Marks wrote:
>> Right, I remember this issue. In the email you referenced [1] Max said "I
>> remember we agreed on several rules" which were basically to use diamond if
>> it's an initializer at the point of declaration. I guess it depends on who
>> "we" are. I recall that discussion occurring with the security team (of which
>> Max is a member) so I thought "we" meant the security team. (Perhaps the same
>> approach was applied to networking changes as well.) However, I had also
>> applied changesets outside the security area that used diamond much more
>> extensively. So, unfortunately, different areas of the code are using
>> different conventions.
>>
>> Personally I don't see any problem with using diamond in initializers,
>> assignment statements (separate from the declaration), and return statements.
>> I wrote a blog about this: [2].
>>
>> For RMI at least, I'd prefer to see diamond used in the places that I
>> recommended.
>>
>> s'marks
>>
>>
>> [1] http://mail.openjdk.java.net/pipermail/net-dev/2011-September/003547.html
>>
>> [2] http://stuartmarks.wordpress.com/2011/04/29/when-should-diamond-be-used/
>>
>>
>>
>> On 2/28/12 3:22 PM, Kurchi Hazra wrote:
>>> Hi Stuart,
>>>
>>> Thanks for your comments. Regarding the use of diamonds, I remember this issue
>>> coming up when i was
>>> fixing networking warnings. See :
>>> http://mail.openjdk.java.net/pipermail/net-dev/2011-September/003547.html
>>>
>>> We had stuck to using diamond only when both declaration and assignment were on
>>> the same line.
>>>
>>> - Kurchi
>>>
>>>
>>>
>>> On 2/28/2012 3:08 PM, Stuart Marks wrote:
>>>> Hi Kurchi,
>>>>
>>>> I looked at the rest of the files. Pretty good, taking on diamond,
>>>> multi-catch, and try-with-resources as well!
>>>>
>>>> I have several comments. Mostly nitpicks, but a few items worthy of some
>>>> discussion, but overall still minor changes, I think.
>>>>
>>>>
>>>> com/sun/rmi/rmid/ExecOptionPermission.java:
>>>>
>>>> - L234 can use diamond
>>>>
>>>>
>>>> com/sun/rmi/rmid/ExecPermission.java:
>>>>
>>>> - L238 can use diamond
>>>>
>>>>
>>>> sun/rmi/rmic/Main.java:
>>>>
>>>> - L186, L194 can use diamond
>>>>
>>>> - At L83, the type of environmentClass can be changed to Class<? extends
>>>> BatchEnvironment>. The assignment to environmentClass at L426 would need to
>>>> be replaced with a call to BatchEnvironment.class.asSubclass() in order to
>>>> get the types to work out. (The call to isAssignableFrom() should remain
>>>> since it's checking against the current value of environmentClass, not
>>>> BatchEnvironment.class.) Then, the Constructor declaration at L498 should
>>>> change to Constructor<? extends BatchEnvironment> and then the cast at L499
>>>> can go away.
>>>>
>>>>
>>>> sun/rmi/rmic/RMIGenerator.java:
>>>>
>>>> - L686 is now short enough to be joined to the previous line.
>>>>
>>>>
>>>> sun/rmi/server/ActivatableRef.java:
>>>>
>>>> - L377 indentation should shift to match with previous line.
>>>>
>>>>
>>>> sun/rmi/transport/ConnectionInputStream.java:
>>>>
>>>> - L91-100: Ugh! The addition of generics here makes this really bad. Not your
>>>> fault; you've added generics in the precise, minimal way. But this code just
>>>> cries out to be simplified. This is usually out of bounds for warnings
>>>> changes but since I'm closer to this code I'd say to go ahead with it. Go
>>>> ahead and replace this with an enhanced for loop and get rid of some
>>>> intermediate locals:
>>>>
>>>> void registerRefs() throws IOException {
>>>> if (!incomingRefTable.isEmpty()) {
>>>> for (Map.Entry<Endpoint, List<LiveRef>> entry :
>>>> incomingRefTable.entrySet()) {
>>>> DGCClient.registerRefs(entry.getKey(), entry.getValue());
>>>> }
>>>> }
>>>> }
>>>>
>>>>
>>>> sun/rmi/transport/DGCClient.java:
>>>>
>>>> - L285, L606, L611: use diamond
>>>> - L690: remove redundant parentheses
>>>>
>>>>
>>>> sun/rmi/transport/StreamRemoteCall.java:
>>>>
>>>> I think it would be better to put the comment about "fall through" at line
>>>> 253 or 256 instead of at the top of the method (L201) which is pretty far
>>>> away. The point here is that exceptionReceivedFromServer() always throws an
>>>> exception -- well, it should -- and thus this case cannot fall through to the
>>>> default case. This isn't obvious, so I'd prefer to see a comment somewhere
>>>> near here instead of at the top of the method.
>>>>
>>>> (One might ask, can't the compiler determine that the method always throws an
>>>> exception, which means the case can't fall through, and thus shut off the
>>>> fall through warning? Well, the method in question is protected, so a
>>>> subclass might override the method and not always throw an exception. That
>>>> would be a bug, but the compiler can't tell that. (Tom Hawtin suggests that
>>>> it's bad style for a method always to throw an exception, and instead that it
>>>> should return an exception that the caller is responsible for throwing. This
>>>> would make the code clearer. (This has come up in prior warnings cleanups;
>>>> see [1]. (Changing this is usually out of scope for warnings cleanup, though.
>>>> I'm tempted to ask you to change this, but some tests are looking for
>>>> exceptionReceivedFromServer in stack traces and it's probably not worth the
>>>> risk of messing them up. (Yes, I'm using too many nested parentheses.)))))
>>>>
>>>> [1]
>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-December/008524.html
>>>>
>>>>
>>>> sun/rmi/transport/proxy/RMIMasterSocketFactory.java:
>>>>
>>>> - L99 can use diamond
>>>>
>>>> - L240, hmm, refactoring to use try-with-resources. Note that the original
>>>> code leaks the socket if read() throws IOException! Overall using t-w-r is
>>>> good, and fixes this bug, but it can be improved further. Probably move the
>>>> "trying with factory" log message outside of the try block at L230, and turn
>>>> that try block into the try-with-resources, instead of adding a nested t-w-r.
>>>> The semantics are almost the same, as the close() is performed before any
>>>> IOException is caught. (This kind of change is usually out of bounds for
>>>> warnings changes but, as above, since I'm closer to this code I'd say to go
>>>> ahead with it.)
>>>>
>>>> Thanks,
>>>>
>>>> s'marks
>>>>
>>>> On 2/24/12 2:24 PM, Kurchi Hazra wrote:
>>>>> Hi,
>>>>>
>>>>> Please ignore the previous webrev and see this instead:
>>>>> http://cr.openjdk.java.net/~khazra/7146763/webrev.03/
>>>>>
>>>>> This has Stuart's suggestion integrated correctly. In addition, I realized
>>>>> that
>>>>> make/sun/rmi/rmic/Makefile is not yet ready to have the JAVAC_WARNINGS_FATAL
>>>>> flag turned on, since it implicitly also builds files from sun/tools with
>>>>> more
>>>>> then 400
>>>>> warnings in them. The change in this file has now been removed.
>>>>>
>>>>> - Kurchi
>>>>>
>>>>>
>>>>>
>>>>> On 2/24/2012 11:01 AM, Kurchi Hazra wrote:
>>>>>> Hi Stuart,
>>>>>>
>>>>>> Thanks for the detailed explanation. Here is an updated webrev:
>>>>>> http://cr.openjdk.java.net/~khazra/7146763/webrev.02/
>>>>>>
>>>>>>
>>>>>> - Kurchi
>>>>>>
>>>>>> On 2/24/2012 12:54 AM, Stuart Marks wrote:
>>>>>>> On 2/22/12 1:25 PM, Kurchi Hazra wrote:
>>>>>>>> On 2/22/2012 10:01 AM, Rémi Forax wrote:
>>>>>>>>> Hi Kurchi, hi all,
>>>>>>>>>
>>>>>>>>> in ReliableLog, you can get ride of the @SupressWarnings,
>>>>>>>>> getLogClassConstructor should return a Constructor<?> and not a
>>>>>>>>> Constructor<?
>>>>>>>>> extends LogFile>,
>>>>>>>>> the field logClassConstructor should be typed Constructor<?> and
>>>>>>>>> in openLogFile, the log should be constructed like this
>>>>>>>>>
>>>>>>>>> log = (logClassConstructor == null ?
>>>>>>>>> new LogFile(logName, "rw") :
>>>>>>>>> (LogFile)logClassConstructor.newInstance(logName, "rw"));
>>>>>>>>>
>>>>>>>>> The idea is that a cast on a LogFile is typesafe but not a cast on a
>>>>>>>>> Constructor<? extends LogFile>.
>>>>>>>>
>>>>>>>> If I change the return type to Constructor<?>, I get the following error:
>>>>>>>> ../../../../src/share/classes/sun/rmi/log/ReliableLog.java:122: error:
>>>>>>>> incompatible types
>>>>>>>> logClassConstructor = getLogClassConstructor();
>>>>>>>> ^
>>>>>>>> required: Constructor<? extends LogFile>
>>>>>>>> found: Constructor<CAP#1>
>>>>>>>> where CAP#1 is a fresh type-variable:
>>>>>>>> CAP#1 extends Object from capture of ?
>>>>>>>> And the following warning:
>>>>>>>>
>>>>>>>> ../../../../src/share/classes/sun/rmi/log/ReliableLog.java:350: warning:
>>>>>>>> [unchecked] unchecked cast
>>>>>>>> cl.getConstructor(String.class, String.class);
>>>>>>>> ^
>>>>>>>> required: Constructor<? extends LogFile>
>>>>>>>> found: Constructor<CAP#1>
>>>>>>>> where CAP#1 is a fresh type-variable:
>>>>>>>> CAP#1 extends Object from capture of ?
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Kurchi
>>>>>>>
>>>>>>> Hi Kurchi,
>>>>>>>
>>>>>>> To implement Rémi's suggestion fully, you would also have to change the
>>>>>>> type
>>>>>>> of logClassConstructor to Contructor<?> near line 122, remove the cast of
>>>>>>> cl.getConstructor() near line 350, and then add the cast to LogFile at the
>>>>>>> call to newInstance() near line 546.
>>>>>>>
>>>>>>> This works to get rid of the warnings and errors, but the declaration of
>>>>>>> Constructor<?> is somewhat imprecise. The code checks to make sure that the
>>>>>>> loaded class is a subclass of LogFile (that's what the isAssignableFrom
>>>>>>> check is doing). Thus the type of the loaded class really should be Class<?
>>>>>>> extends LogFile>, and correspondingly the logClassConstructor should be
>>>>>>> Constructor<? extends LogFile>. That's how logClassConstructor is declared
>>>>>>> now and it would be nice to keep it that way.
>>>>>>>
>>>>>>> It turns out that Class.asSubclass() does this conversion without
>>>>>>> generating
>>>>>>> an unchecked warning. This internally does an isAssignableFrom() check and
>>>>>>> casts to the right wildcarded type, so this can simplify the code in
>>>>>>> getLogClassConstructor() somewhat as well. (Incidentally, asSubClass() has
>>>>>>> @SuppressWarnings on its implementation.) I've appended some diffs below
>>>>>>> (to
>>>>>>> be applied on top of your most recent webrev) to show how this can be done.
>>>>>>>
>>>>>>> The behavior is slightly different, as it throws ClassCastException (which
>>>>>>> is caught by the catch clause below, emitting a log message) instead of
>>>>>>> silently returning null. This is probably an improvement, since if the user
>>>>>>> specifies the wrong class in the property name, the exception stack trace
>>>>>>> should indicate what happened.
>>>>>>>
>>>>>>> s'marks
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> diff -r 72d32fd57d89 src/share/classes/sun/rmi/log/ReliableLog.java
>>>>>>> --- a/src/share/classes/sun/rmi/log/ReliableLog.java Fri Feb 24 00:01:53
>>>>>>> 2012 -0800
>>>>>>> +++ b/src/share/classes/sun/rmi/log/ReliableLog.java Fri Feb 24 00:39:02
>>>>>>> 2012 -0800
>>>>>>> @@ -330,9 +330,7 @@
>>>>>>> * property a) can be loaded, b) is a subclass of LogFile, and c) has a
>>>>>>> * public two-arg constructor (String, String); otherwise returns null.
>>>>>>> **/
>>>>>>> - @SuppressWarnings("unchecked")
>>>>>>> - private static Constructor<? extends LogFile>
>>>>>>> - getLogClassConstructor() {
>>>>>>> + private static Constructor<? extends LogFile> getLogClassConstructor() {
>>>>>>>
>>>>>>> String logClassName = AccessController.doPrivileged(
>>>>>>> new GetPropertyAction("sun.rmi.log.class"));
>>>>>>> @@ -345,11 +343,9 @@
>>>>>>> return ClassLoader.getSystemClassLoader();
>>>>>>> }
>>>>>>> });
>>>>>>> - Class<?> cl = loader.loadClass(logClassName);
>>>>>>> - if (LogFile.class.isAssignableFrom(cl)) {
>>>>>>> - return (Constructor<? extends LogFile>)
>>>>>>> - cl.getConstructor(String.class, String.class);
>>>>>>> - }
>>>>>>> + Class<? extends LogFile> cl =
>>>>>>> + loader.loadClass(logClassName).asSubclass(LogFile.class);
>>>>>>> + return cl.getConstructor(String.class, String.class);
>>>>>>> } catch (Exception e) {
>>>>>>> System.err.println("Exception occurred:");
>>>>>>> e.printStackTrace();
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>