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