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

Wed Feb 29 00:32:16 UTC 2012

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();
>>>>>
>>>>>
>>>>
>>>
>