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
