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.
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, /li 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:
*bConfiguration:/b
* By default eachttConsoleHandler/tt is initialized using
the following
*ttLogManager/tt configuration properties. If properties are
not defined
* (or have invalid values) then the specified default values are
used.
*ul
*li handler's classname.level
*specifies the default level for thettHandler/tt
*(defaults tottLevel.INFO/tt).
...snip
*/ul
*
* For example, the properties for {@code ConsoleHandler} would be:
* java.util.logging.ConsoleHandler.level=INFO
*
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, /li 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: *bConfiguration:/b * By default eachttConsoleHandler/tt is initialized using the following *ttLogManager/tt configuration properties. If properties are not
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
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
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
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
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
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-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
Fine and Dandy :-) ...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-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
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: 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 ttExecutionException/tt with the specified cause.
- * The detail message is set to:
- * pre
- * (cause == null ? null : cause.toString())/pre
- * (which typically contains the class and detail message of
- * ttcause/tt).
+ * The detail message is set to {@code (cause == null ? null :
+ * cause.toString())} (which typically contains the class and
+ * detail message of ttcause/tt).
The last ttcause/tt should be replaced by {@Code cause}.
There are a couple of places that refer to JLS (eg package-info, Locks.java)
+ * a href=http://java.sun.com/docs/books/jls/; The Java Language
+ * Specification, Third Edition (17.4 Memory Model)/a:
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-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
