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

Kurchi Hazra kurchi.subhra.hazra at oracle.com
Tue Feb 28 23:22:47 UTC 2012


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

-- 
-Kurchi




More information about the core-libs-dev mailing list