Re: Review Request - JDK-4239752: FileSystem should be a platform-specific class to avoid native code
Looks good to me. I have to wonder why we ever used a native method to invoke a Java constructor though ??? David On 26/10/2012 7:26 AM, Dan Xu wrote: Hi, Please review the code change to avoid native codes when creating the FileSystem object, http://cr.openjdk.java.net/~dxu/4239752/webrev/. In the change, the native codes for windows and unix platforms are removed. Instead, corresponding Java codes are added for each platform. Thanks! -Dan
Re: Minor/sync/cleanup j.u.c with Dougs CVS - Oct 2012
Hi Chris, You can count me as a reviewer anyway. :) A couple of observations: --- old/src/share/classes/java/util/concurrent/ExecutionException.java Thu Oct 25 14:14:15 2012 +++ new/src/share/classes/java/util/concurrent/ExecutionException.java Thu Oct 25 14:14:14 2012 @@ -79,11 +79,9 @@ /** * Constructs an ExecutionException with the specified cause. - * The detail message is set to: - * - * (cause == null ? null : cause.toString()) - * (which typically contains the class and detail message of - * cause). + * The detail message is set to {@code (cause == null ? null : + * cause.toString())} (which typically contains the class and + * detail message of cause). The last cause should be replaced by {@Code cause}. There are a couple of places that refer to JLS (eg package-info, Locks.java) + * http://java.sun.com/docs/books/jls/";> The Java Language + * Specification, Third Edition (17.4 Memory Model): which should, I believe, be updated to: http://docs.oracle.com/javase/specs/jls/se7/html/index.html Thanks, David On 26/10/2012 12:13 AM, Chris Hegarty wrote: In preparation to a re-sync of the java.util.concurrent classes with Doug's CVS, I've extracted most of the minor/small changes. This will reduce the noise when reviewing the remainder of the implementation changes. More specifically, Cleanup: javadoc style/consistency javadoc example code style imports whitespace uniform serialization method javadoc typos Minor/small impl changes: remove redundant null checks throw NPE when more efficient rework timeouts, lasttime -> deadline STPE, make drainTo methods more robust when add throws To be clear, I'm not requested a review here. These are Doug's changes are I am already a reviewer, but please feel free ( be warned, nothing interesting here! ) http://cr.openjdk.java.net/~chegar/8001575/webrev.00/webrev/ -Chris.
Re: Review Request: JDK-8001565, (fs) Typo Path.endsWith(String) javadoc
I have your patch from the webrev. Mandy On 10/25/2012 3:17 PM, Dan Xu wrote: Thank you. Do you need me to generate the patch? -Dan On 10/25/2012 03:12 PM, Mandy Chung wrote: On 10/25/2012 2:45 PM, Dan Xu wrote: Hi, Please help review the javadoc typo fix at, http://cr.openjdk.java.net/~dxu/8001565/webrev/. Thanks! Looks good with me. I can push this for you. Mandy
Re: Review Request: JDK-8001565, (fs) Typo Path.endsWith(String) javadoc
Thank you. Do you need me to generate the patch? -Dan On 10/25/2012 03:12 PM, Mandy Chung wrote: On 10/25/2012 2:45 PM, Dan Xu wrote: Hi, Please help review the javadoc typo fix at, http://cr.openjdk.java.net/~dxu/8001565/webrev/. Thanks! Looks good with me. I can push this for you. Mandy
Re: Review Request: JDK-8001565, (fs) Typo Path.endsWith(String) javadoc
Fine and "Dan"dy :-) ...Jim On 10/25/2012 05:45 PM, Dan Xu wrote: Hi, Please help review the javadoc typo fix at, http://cr.openjdk.java.net/~dxu/8001565/webrev/. Thanks! -Dan -- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.g...@oracle.com
Re: Review Request - JDK-4239752: FileSystem should be a platform-specific class to avoid native code
Looks good to me. Jim On 10/25/2012 05:26 PM, Dan Xu wrote: Hi, Please review the code change to avoid native codes when creating the FileSystem object, http://cr.openjdk.java.net/~dxu/4239752/webrev/. In the change, the native codes for windows and unix platforms are removed. Instead, corresponding Java codes are added for each platform. Thanks! -Dan -- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.g...@oracle.com
Re: Review Request: JDK-8001565, (fs) Typo Path.endsWith(String) javadoc
On 10/25/2012 2:45 PM, Dan Xu wrote: Hi, Please help review the javadoc typo fix at, http://cr.openjdk.java.net/~dxu/8001565/webrev/. Thanks! Looks good with me. I can push this for you. Mandy
Re: Review Request: JDK-8001565, (fs) Typo Path.endsWith(String) javadoc
This fix is fine Dan Best Lance On Oct 25, 2012, at 5:45 PM, Dan Xu wrote: > Hi, > > Please help review the javadoc typo fix at, > http://cr.openjdk.java.net/~dxu/8001565/webrev/. Thanks! > > -Dan > Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Review Request: JDK-8001565, (fs) Typo Path.endsWith(String) javadoc
Hi, Please help review the javadoc typo fix at, http://cr.openjdk.java.net/~dxu/8001565/webrev/. Thanks! -Dan
Review Request - JDK-4239752: FileSystem should be a platform-specific class to avoid native code
Hi, Please review the code change to avoid native codes when creating the FileSystem object, http://cr.openjdk.java.net/~dxu/4239752/webrev/. In the change, the native codes for windows and unix platforms are removed. Instead, corresponding Java codes are added for each platform. Thanks! -Dan
hg: jdk8/tl/langtools: 6725230: Java Compilation with Jsr199 ignores Class-Path in manifest
Changeset: ea2616a6bd01 Author:jjg Date: 2012-10-25 13:33 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/ea2616a6bd01 6725230: Java Compilation with Jsr199 ignores Class-Path in manifest Reviewed-by: jjg, mcimadamore Contributed-by: vicente.rom...@oracle.com ! src/share/classes/com/sun/tools/javac/file/Locations.java + test/tools/javac/Paths/TestCompileJARInClassPath.java
hg: jdk8/tl/langtools: 7200915: convert TypeTags from a series of small ints to an enum
Changeset: c002fdee76fd Author:jjg Date: 2012-10-25 11:09 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/c002fdee76fd 7200915: convert TypeTags from a series of small ints to an enum Reviewed-by: jjg, mcimadamore Contributed-by: vicente.rom...@oracle.com ! src/share/classes/com/sun/tools/javac/code/Attribute.java ! src/share/classes/com/sun/tools/javac/code/Kinds.java ! src/share/classes/com/sun/tools/javac/code/Printer.java ! src/share/classes/com/sun/tools/javac/code/Symbol.java ! src/share/classes/com/sun/tools/javac/code/Symtab.java ! src/share/classes/com/sun/tools/javac/code/Type.java + src/share/classes/com/sun/tools/javac/code/TypeTag.java - src/share/classes/com/sun/tools/javac/code/TypeTags.java ! src/share/classes/com/sun/tools/javac/code/Types.java ! src/share/classes/com/sun/tools/javac/comp/Annotate.java ! src/share/classes/com/sun/tools/javac/comp/Attr.java ! src/share/classes/com/sun/tools/javac/comp/Check.java ! src/share/classes/com/sun/tools/javac/comp/ConstFold.java ! src/share/classes/com/sun/tools/javac/comp/DeferredAttr.java ! src/share/classes/com/sun/tools/javac/comp/Enter.java ! src/share/classes/com/sun/tools/javac/comp/Flow.java ! src/share/classes/com/sun/tools/javac/comp/Infer.java ! src/share/classes/com/sun/tools/javac/comp/Lower.java ! src/share/classes/com/sun/tools/javac/comp/MemberEnter.java ! src/share/classes/com/sun/tools/javac/comp/Resolve.java ! src/share/classes/com/sun/tools/javac/comp/TransTypes.java ! src/share/classes/com/sun/tools/javac/jvm/ClassReader.java ! src/share/classes/com/sun/tools/javac/jvm/ClassWriter.java ! src/share/classes/com/sun/tools/javac/jvm/Code.java ! src/share/classes/com/sun/tools/javac/jvm/Gen.java ! src/share/classes/com/sun/tools/javac/jvm/UninitializedType.java ! src/share/classes/com/sun/tools/javac/main/JavaCompiler.java ! src/share/classes/com/sun/tools/javac/model/JavacElements.java ! src/share/classes/com/sun/tools/javac/parser/JavacParser.java ! src/share/classes/com/sun/tools/javac/tree/JCTree.java ! src/share/classes/com/sun/tools/javac/tree/Pretty.java ! src/share/classes/com/sun/tools/javac/tree/TreeInfo.java ! src/share/classes/com/sun/tools/javac/tree/TreeMaker.java ! src/share/classes/com/sun/tools/javac/util/Constants.java ! src/share/classes/com/sun/tools/javac/util/RichDiagnosticFormatter.java ! src/share/classes/com/sun/tools/javadoc/AnnotationValueImpl.java ! src/share/classes/com/sun/tools/javadoc/ClassDocImpl.java ! src/share/classes/com/sun/tools/javadoc/FieldDocImpl.java ! src/share/classes/com/sun/tools/javadoc/MethodDocImpl.java ! src/share/classes/com/sun/tools/javadoc/ParameterizedTypeImpl.java ! src/share/classes/com/sun/tools/javadoc/TypeMaker.java ! test/tools/javac/6889255/T6889255.java ! test/tools/javac/tree/MakeLiteralTest.java
Re: RFR: 7159567 - inconsistent configuration of MemoryHandler
On 10/25/2012 10:25 AM, Jim Gish wrote: OK. One more time. http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler Great, thanks! I'll push it for you. I compromised with the RuntimeException. I'm instead catching it, but throwing a new one this way: 65 throw new RuntimeException( 66 "Test Failed: did not load java.util.logging.ConsoleHandler as expected", 67 rte); That way, we retain the original, but have the advantage of having a clear indication of "Test Failed" and the reason. Otherwise, diagnosing the failure forces the reader to dig into the stack trace. I like a clear error message too. If the test fails, you will need the original exception and the stack trace to diagnose the problem anyway and in some cases, the exception is clear enough. Anyway, just to explain why I had the comment. I'm okay with what you have. Mandy Thanks, Jim On 10/24/2012 08:40 PM, Mandy Chung wrote: On 10/24/2012 12:31 PM, Jim Gish wrote: See updated webrev: http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler Looks good. Thanks for the update. MemoryHandlerTest.java - thanks for renaming it but you forget to change L28 @run tag. jtreg should fail if you run this new test. L64-66 this try-catch block isn't necessary, as I mentioned in my previous comment, but no big deal if you want to leave it there. The comment lines and some throw statements are really long and should be broken into multiple lines (I didn't notice the long lines in previous versions - sorry if I had missed them). Hopefully it's just one-click reformat for you using IDE :) Mandy Thanks, Jim On 10/17/2012 03:46 PM, Mandy Chung wrote: Hi Jim, On 10/11/2012 2:37 PM, Jim Gish wrote: Please review the updated changes at http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler/ The spec change looks good. As Alan points out, is missing. Although they were not there before, I would think it's a good clean up while you are in these files if you agree. Done The test looks better. Is SimpleTargetHandler.numPublished intended to be checked? SimpleTargetHandler is set as the target for java.util.logging.MemoryHandler. I guess you want to create a logger using the standard MemoryHandler. Nit: the test is named MemoryHandler and I guess the name conflict causes the custom handler classes to extend "java.util.logging.MemoryHandler" with a fully-qualified class name. As the properties file is named MemoryHandlerTest.props, do you consider renaming the test to MemoryHandlerTest to avoid the name conflict? I don't have strong opinion and just want to point that out. I don't see this as a problem. However, I've renamed MemoryHandler to MemoryHandlerTest for improved clarity. L62-64: better not to rethrow a new RuntimeException as the exception and stack trace will help diagnostics if it does go wrong. You can get rid of this try-catch block. OK -- the reason I did this was to insert a readable message into the new RuntimeException to indicate the cause of the failure. I think this is good practice and leads to easier diagnosis, but since you disagree, I'll take it out. L120: is it a leftover debug statement? I think you meant to add test case to exercise this target handler, right? removed and a few tests added. Jim I've changed as you've requested, added some additional tests, did some better error handling in the case of a MemoryHandler not specifying a target (now throws RuntimeException with an appropriate message instead of attempting to load a null class and throwing NPE). I also corrected the copyrights, tested with JCK, all jdk_lang tests and have submitted a JPRT job with core tests. Great. Thanks for doing it. Mandy I've forwarded a CCC request (separately) and will await its approval and further review of this change. Thanks, Jim On 09/28/2012 05:32 PM, Mandy Chung wrote: On 9/28/2012 12:13 PM, Jim Gish wrote: I've re-spun the change with additional usage notes in the spec to reflect the long-standing actual behavior. Note that it doesn't change the spec per se, as it was already stated in LogManager. This change simply replicates that language with an example in each *Handler class to make it easier to find. Thanks for looking into it. This statement in LogManager does specify the properties for handlers: The properties for loggers and Handlers will have names starting with the dot-separated name for the handler or logger. Replicating that statement with an example is one way to do it. Would it be clearer if the prefix of the properties referenced in the bullet list is replaced from "java.util.logging" to some kind of prefix - something like this: *Configuration: * By default eachConsoleHandler is initialized using the following *LogManager configuration properties. If properties are not defined
Re: RFR: 7159567 - inconsistent configuration of MemoryHandler
OK. One more time. http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler I compromised with the RuntimeException. I'm instead catching it, but throwing a new one this way: 65 throw new RuntimeException( 66 "Test Failed: did not load java.util.logging.ConsoleHandler as expected", 67 rte); That way, we retain the original, but have the advantage of having a clear indication of "Test Failed" and the reason. Otherwise, diagnosing the failure forces the reader to dig into the stack trace. Thanks, Jim On 10/24/2012 08:40 PM, Mandy Chung wrote: On 10/24/2012 12:31 PM, Jim Gish wrote: See updated webrev: http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler Looks good. Thanks for the update. MemoryHandlerTest.java - thanks for renaming it but you forget to change L28 @run tag. jtreg should fail if you run this new test. L64-66 this try-catch block isn't necessary, as I mentioned in my previous comment, but no big deal if you want to leave it there. The comment lines and some throw statements are really long and should be broken into multiple lines (I didn't notice the long lines in previous versions - sorry if I had missed them). Hopefully it's just one-click reformat for you using IDE :) Mandy Thanks, Jim On 10/17/2012 03:46 PM, Mandy Chung wrote: Hi Jim, On 10/11/2012 2:37 PM, Jim Gish wrote: Please review the updated changes at http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler/ The spec change looks good. As Alan points out, is missing. Although they were not there before, I would think it's a good clean up while you are in these files if you agree. Done The test looks better. Is SimpleTargetHandler.numPublished intended to be checked? SimpleTargetHandler is set as the target for java.util.logging.MemoryHandler. I guess you want to create a logger using the standard MemoryHandler. Nit: the test is named MemoryHandler and I guess the name conflict causes the custom handler classes to extend "java.util.logging.MemoryHandler" with a fully-qualified class name. As the properties file is named MemoryHandlerTest.props, do you consider renaming the test to MemoryHandlerTest to avoid the name conflict? I don't have strong opinion and just want to point that out. I don't see this as a problem. However, I've renamed MemoryHandler to MemoryHandlerTest for improved clarity. L62-64: better not to rethrow a new RuntimeException as the exception and stack trace will help diagnostics if it does go wrong. You can get rid of this try-catch block. OK -- the reason I did this was to insert a readable message into the new RuntimeException to indicate the cause of the failure. I think this is good practice and leads to easier diagnosis, but since you disagree, I'll take it out. L120: is it a leftover debug statement? I think you meant to add test case to exercise this target handler, right? removed and a few tests added. Jim I've changed as you've requested, added some additional tests, did some better error handling in the case of a MemoryHandler not specifying a target (now throws RuntimeException with an appropriate message instead of attempting to load a null class and throwing NPE). I also corrected the copyrights, tested with JCK, all jdk_lang tests and have submitted a JPRT job with core tests. Great. Thanks for doing it. Mandy I've forwarded a CCC request (separately) and will await its approval and further review of this change. Thanks, Jim On 09/28/2012 05:32 PM, Mandy Chung wrote: On 9/28/2012 12:13 PM, Jim Gish wrote: I've re-spun the change with additional usage notes in the spec to reflect the long-standing actual behavior. Note that it doesn't change the spec per se, as it was already stated in LogManager. This change simply replicates that language with an example in each *Handler class to make it easier to find. Thanks for looking into it. This statement in LogManager does specify the properties for handlers: The properties for loggers and Handlers will have names starting with the dot-separated name for the handler or logger. Replicating that statement with an example is one way to do it. Would it be clearer if the prefix of the properties referenced in the bullet list is replaced from "java.util.logging" to some kind of prefix - something like this: *Configuration: * By default eachConsoleHandler is initialized using the following *LogManager configuration properties. If properties are not defined * (or have invalid values) then the specified default values are used. * * .level *specifies the default level for theHandler *(defaults toLevel.INFO). ... * * * For example, the properties for {@code ConsoleHandler} would be: * java.util.logging.ConsoleHandler.level=INFO * java.util.logging.ConsoleHandler.formatter=java.util.logging.S
Minor/sync/cleanup j.u.c with Dougs CVS - Oct 2012
In preparation to a re-sync of the java.util.concurrent classes with Doug's CVS, I've extracted most of the minor/small changes. This will reduce the noise when reviewing the remainder of the implementation changes. More specifically, Cleanup: javadoc style/consistency javadoc example code style imports whitespace uniform serialization method javadoc typos Minor/small impl changes: remove redundant null checks throw NPE when more efficient rework timeouts, lasttime -> deadline STPE, make drainTo methods more robust when add throws To be clear, I'm not requested a review here. These are Doug's changes are I am already a reviewer, but please feel free ( be warned, nothing interesting here! ) http://cr.openjdk.java.net/~chegar/8001575/webrev.00/webrev/ -Chris.