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

Thu Mar 1 17:48:54 UTC 2012

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

-- 
-Kurchi