Code review request: 7088502 Security libraries don't build with javac -Werror
Hi, The following webrev removes warnings in sun.rmi.* packages. I have neglected nearly all deprecation warnings, since this code uses deprecated classes such as java.rmi.server.LogStream with no suggested replacements. I have included -Xlint:all,-deprecation as an option instead in the appropriate Makefiles. Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7146763 Webrev: http://cr.openjdk.java.net/~khazra/7146763/webrev.00/ Thanks, Kurchi
Corrected the subject line. Hi, The following webrev removes warnings in sun.rmi.* packages. I have neglected nearly all deprecation warnings, since this code uses deprecated classes such as java.rmi.server.LogStream with no suggested replacements. I have included -Xlint:all,-deprecation as an option instead in the appropriate Makefiles. Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7146763 Webrev: http://cr.openjdk.java.net/~khazra/7146763/webrev.00/ Thanks, Kurchi
Kurchi, Great work. I've been through the complete webrev and I think it looks good. Just a few minor comments: - there are API changes, but only in sun private implementation classes, so this should be fine. - Minor indentation nit where method declaration return type was generified. The method args on subsequent lines should be equally indented. Example LoaderHandler.java L152: public static Class<?> loadClass(String codebase, String name, >>>> ClassLoader defaultLoader) - There are opportunities to use auto boxing/unboxing >: diff RMIGenerator.java 99c99 < version = versionOptions.get(arg); --- > version = (versionOptions.get(arg)).intValue(); ConnectionMultiplexer.java >: diff ConnectionMultiplexer.java ConnectionMultiplexer.java.1 133a134 < Integer idObj; 150c151,152 > info = connectionTable.get(id); --- < idObj = new Integer(id); < info = connectionTable.get(idObj); 158c160 > connectionTable.put(id, info); --- < connectionTable.put(idObj, info); ..... -Chris. On 22/02/2012 05:50, Kurchi Hazra wrote:
Corrected the subject line.
Hi,
The following webrev removes warnings in sun.rmi.* packages. I have neglected nearly all deprecation warnings, since this code uses deprecated classes such as java.rmi.server.LogStream with no suggested replacements. I have included -Xlint:all,-deprecation as an option instead in the appropriate Makefiles.
Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7146763 Webrev: http://cr.openjdk.java.net/~khazra/7146763/webrev.00/
Thanks, Kurchi
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>. In the same file, the try-with-resources is not correcly used try (DataOutputStream out = new DataOutputStream(new FileOutputStream(fName(name)))) { writeInt(out, version); } should be try (FileOutputStream fos = new FileOutputStream(fName(name)); DataOutputStream out = new DataOutputStream(fos)) { writeInt(out, version); } because if new DataOutputStream throws an exception, the FileOutputStream will not be closed. Basically, all stream filters should have it's own line in a try-with-resources. cheers, Rémi On 02/22/2012 12:16 PM, Chris Hegarty wrote:
Kurchi,
Great work. I've been through the complete webrev and I think it looks good. Just a few minor comments:
- there are API changes, but only in sun private implementation classes, so this should be fine.
- Minor indentation nit where method declaration return type was generified. The method args on subsequent lines should be equally indented. Example LoaderHandler.java L152:
public static Class<?> loadClass(String codebase, String name,
ClassLoader defaultLoader)
- There are opportunities to use auto boxing/unboxing
: diff RMIGenerator.java 99c99 < version = versionOptions.get(arg);
version = (versionOptions.get(arg)).intValue();
ConnectionMultiplexer.java
: diff ConnectionMultiplexer.java ConnectionMultiplexer.java.1 133a134 < Integer idObj; 150c151,152 info = connectionTable.get(id);
< idObj = new Integer(id); < info = connectionTable.get(idObj); 158c160
connectionTable.put(id, info);
--- < connectionTable.put(idObj, info); .....
-Chris.
On 22/02/2012 05:50, Kurchi Hazra wrote:
Corrected the subject line.
Hi,
The following webrev removes warnings in sun.rmi.* packages. I have neglected nearly all deprecation warnings, since this code uses deprecated classes such as java.rmi.server.LogStream with no suggested replacements. I have included -Xlint:all,-deprecation as an option instead in the appropriate Makefiles.
Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7146763 Webrev: http://cr.openjdk.java.net/~khazra/7146763/webrev.00/
Thanks, Kurchi
Hi Remi, Thanks for your review. Please see my comment: 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
In the same file, the try-with-resources is not correcly used
try (DataOutputStream out = new DataOutputStream(new FileOutputStream(fName(name)))) { writeInt(out, version); }
should be
try (FileOutputStream fos = new FileOutputStream(fName(name)); DataOutputStream out = new DataOutputStream(fos)) { writeInt(out, version); }
because if new DataOutputStream throws an exception, the FileOutputStream will not be closed. Basically, all stream filters should have it's own line in a try-with-resources.
cheers, Rémi
On 02/22/2012 12:16 PM, Chris Hegarty wrote:
Kurchi,
Great work. I've been through the complete webrev and I think it looks good. Just a few minor comments:
- there are API changes, but only in sun private implementation classes, so this should be fine.
- Minor indentation nit where method declaration return type was generified. The method args on subsequent lines should be equally indented. Example LoaderHandler.java L152:
public static Class<?> loadClass(String codebase, String name,
ClassLoader defaultLoader)
- There are opportunities to use auto boxing/unboxing
: diff RMIGenerator.java 99c99 < version = versionOptions.get(arg);
version = (versionOptions.get(arg)).intValue();
ConnectionMultiplexer.java
: diff ConnectionMultiplexer.java ConnectionMultiplexer.java.1 133a134 < Integer idObj; 150c151,152 info = connectionTable.get(id);
< idObj = new Integer(id); < info = connectionTable.get(idObj); 158c160
connectionTable.put(id, info);
--- < connectionTable.put(idObj, info); .....
-Chris.
On 22/02/2012 05:50, Kurchi Hazra wrote:
Corrected the subject line.
Hi,
The following webrev removes warnings in sun.rmi.* packages. I have neglected nearly all deprecation warnings, since this code uses deprecated classes such as java.rmi.server.LogStream with no suggested replacements. I have included -Xlint:all,-deprecation as an option instead in the appropriate Makefiles.
Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7146763 Webrev: http://cr.openjdk.java.net/~khazra/7146763/webrev.00/
Thanks, Kurchi
-- -Kurchi
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();
On 02/24/2012 09: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
Hi Stuart, hi Kurchi, sorry to not have answer before, and yes, using asSubClass is better that what I've proposed. cheers, Rémi
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
On 02/24/2012 08:01 PM, Kurchi Hazra wrote:
Hi Stuart,
Thanks for the detailed explanation. Here is an updated webrev: http://cr.openjdk.java.net/~khazra/7146763/webrev.02/
Hi Kurchi, the field logClassConstructor should not be changed after all, it should be declared as a Constructor<? extends LogFile> so you can remove the cast to LogFile at line 542. with this change, the patch looks good to me.
- Kurchi
cheers, Rémi
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
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.ht... 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();
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.ht...
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
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.ht...
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();
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.ht...
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
Looks good! Thanks for making the updates. Interestingly, sun/rmi/server/LoaderHandler.java has a couple methods that use Class[] as a parameter (loadProxyClass, loadProxyInterfaces), but which don't generate rawtypes warnings. I may investigate this at some point. Anyway, I think this has gone through enough rounds of review. Go ahead and push. Thanks again. s'marks On 3/1/12 9:48 AM, Kurchi Hazra wrote:
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.ht...
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(); > >
Updated webrev: http://cr.openjdk.java.net/~khazra/7146763/webrev.01/ I have included all changes/suggestion except the Constructor modification that Remi suggested. Darryl: - TCPTransport: Lines #552/553: I actually prefer the way it was as I think it's more readable. RemoteCall is deprecated and I was trying to avoid creating an instance there to remove the resultant Deprecation warning - but I reverted this back - RMIMasterSocketFactory: Line #244: The comment was removed, was there a reason for this? This was a mistake. Changed it now. - Kurchi 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>.
In the same file, the try-with-resources is not correcly used
try (DataOutputStream out = new DataOutputStream(new FileOutputStream(fName(name)))) { writeInt(out, version); }
should be
try (FileOutputStream fos = new FileOutputStream(fName(name)); DataOutputStream out = new DataOutputStream(fos)) { writeInt(out, version); }
because if new DataOutputStream throws an exception, the FileOutputStream will not be closed. Basically, all stream filters should have it's own line in a try-with-resources.
cheers, Rémi
On 02/22/2012 12:16 PM, Chris Hegarty wrote:
Kurchi,
Great work. I've been through the complete webrev and I think it looks good. Just a few minor comments:
- there are API changes, but only in sun private implementation classes, so this should be fine.
- Minor indentation nit where method declaration return type was generified. The method args on subsequent lines should be equally indented. Example LoaderHandler.java L152:
public static Class<?> loadClass(String codebase, String name,
ClassLoader defaultLoader)
- There are opportunities to use auto boxing/unboxing
: diff RMIGenerator.java 99c99 < version = versionOptions.get(arg);
version = (versionOptions.get(arg)).intValue();
ConnectionMultiplexer.java
: diff ConnectionMultiplexer.java ConnectionMultiplexer.java.1 133a134 < Integer idObj; 150c151,152 info = connectionTable.get(id);
< idObj = new Integer(id); < info = connectionTable.get(idObj); 158c160
connectionTable.put(id, info);
--- < connectionTable.put(idObj, info); .....
-Chris.
On 22/02/2012 05:50, Kurchi Hazra wrote:
Corrected the subject line.
Hi,
The following webrev removes warnings in sun.rmi.* packages. I have neglected nearly all deprecation warnings, since this code uses deprecated classes such as java.rmi.server.LogStream with no suggested replacements. I have included -Xlint:all,-deprecation as an option instead in the appropriate Makefiles.
Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7146763 Webrev: http://cr.openjdk.java.net/~khazra/7146763/webrev.00/
Thanks, Kurchi
-- -Kurchi
Hi Kurchi, overall the changes look good. I have a few comments: - TCPTransport: Lines #552/553: I actually prefer the way it was as I think it's more readable. - RMIMasterSocketFactory: Line #244: The comment was removed, was there a reason for this? - HttpInputStream: Line #73: Why even have this if statement? I'd make this a comment instead, something like if contentLengthFound, we should probably do something in this case. - CGIHandler: Line #288: Why even have this if statement? I'd make this a comment instead, something like if contentLengthFound, we should probably do something in this case. Darryl On 02/21/2012 09:50 PM, Kurchi Hazra wrote:
Corrected the subject line.
Hi,
The following webrev removes warnings in sun.rmi.* packages. I have neglected nearly all deprecation warnings, since this code uses deprecated classes such as java.rmi.server.LogStream with no suggested replacements. I have included -Xlint:all,-deprecation as an option instead in the appropriate Makefiles.
Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7146763 Webrev: http://cr.openjdk.java.net/~khazra/7146763/webrev.00/
Thanks, Kurchi
participants (5)
-
Chris Hegarty
-
Darryl Mocek
-
Kurchi Hazra
-
Rémi Forax
-
Stuart Marks