Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader

2012-06-21 Thread Joe Wang
On 6/21/2012 2:00 PM, Alan Bateman wrote: On 21/06/2012 19:02, Joe Wang wrote: : For the class loader, as discussed with Paul, since the ServiceLoader does so much, we may be able to skip the 'using different classloader' part, that is, simply calling load without classloader. The javado

Re: Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException when using a StringBuffer from multiple threads

2012-06-21 Thread Ulf Zibis
Am 21.06.2012 23:41, schrieb Ulf Zibis: Oops crossposting. Looks much better. Maybe you still consider to compare with my suggestions. IMO you do not need to use the "(source)" parts, consistent to the paragraphs above. -Ulf To be consistent to the before paragraph a little more, maybe (1) u

Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader

2012-06-21 Thread Joe Wang
On 6/21/2012 11:04 AM, Alan Bateman wrote: On 21/06/2012 18:34, Joe Wang wrote: : When iterating over the service instances you are catching ConfigurationError 264 ServiceLoader loader = ServiceLoader.load(serviceClass, cl); 265 final Iterator providers = loader.ite

Re: Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException when using a StringBuffer from multiple threads

2012-06-21 Thread Ulf Zibis
Oops crossposting. Looks much better. Maybe you still consider to compare with my suggestions. IMO you do not need to use the "(source)" parts, consistent to the paragraphs above. -Ulf Am 21.06.2012 22:44, schrieb Jim Gish: I did this, and also simplified the language a bit: diff -r 7722e2b

Re: Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException when using a StringBuffer from multiple threads

2012-06-21 Thread Ulf Zibis
Hi again, looking closer, I have some suggestions. The term "create" is not valid code, so should not be tagged as such, same for the separating slashes. The following phrase sounds wrong to me, even I'm not english native speaker: "... the source data passed to create, ... , must ensure that

Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader

2012-06-21 Thread Alan Bateman
On 21/06/2012 19:02, Joe Wang wrote: : For the class loader, as discussed with Paul, since the ServiceLoader does so much, we may be able to skip the 'using different classloader' part, that is, simply calling load without classloader. The javadoc states that it's equivalent to load with co

Re: Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException when using a StringBuffer from multiple threads

2012-06-21 Thread Jim Gish
I did this, and also simplified the language a bit: diff -r 7722e2bec02b src/share/classes/java/lang/StringBuffer.java --- a/src/share/classes/java/lang/StringBuffer.javaThu Jun 21 16:26:04 2012 -0400 +++ b/src/share/classes/java/lang/StringBuffer.javaThu Jun 21 16:42:19 2012 -0400 @@

hg: jdk8/tl/langtools: 7178297: provide mapping from doc comment position to source file position

2012-06-21 Thread jonathan . gibbons
Changeset: 067f51db3402 Author:jjg Date: 2012-06-21 13:22 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/067f51db3402 7178297: provide mapping from doc comment position to source file position Reviewed-by: mcimadamore, ksrini ! src/share/classes/com/sun/tools/javac/par

Re: Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException when using a StringBuffer from multiple threads

2012-06-21 Thread Mike Duigou
Great addition! I believe you should be using {@code} rather than tags. I would say "constructors" rather than "create". I would add the word "operation" after the first instance of "create/append/insert" I would change "This could be done" to "This could be satisfied" Mike On Jun 21 2012,

Re: Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException when using a StringBuffer from multiple threads

2012-06-21 Thread Jim Gish
Taking all the previous comments into consideration, here's an update: diff -r fc575c78f5d3 src/share/classes/java/lang/StringBuffer.java --- a/src/share/classes/java/lang/StringBuffer.javaSun Jun 10 10:29:27 2012 +0100 +++ b/src/share/classes/java/lang/StringBuffer.javaThu Jun 21 14:09

Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader

2012-06-21 Thread Alan Bateman
On 21/06/2012 18:34, Joe Wang wrote: : When iterating over the service instances you are catching ConfigurationError 264 ServiceLoader loader = ServiceLoader.load(serviceClass, cl); 265 final Iterator providers = loader.iterator(); 266 while (providers.hasNex

Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader

2012-06-21 Thread Joe Wang
On 6/21/2012 5:44 AM, Alan Bateman wrote: On 21/06/2012 08:55, Joe Wang wrote: : webrev: http://cr.openjdk.java.net/~joehw/jdk8/7169894/webrev/ It's great to get this work started. The javadoc changes look okay to me and fine for this change. However there are a few areas that could be made

Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader

2012-06-21 Thread Joe Wang
On 6/21/2012 2:35 AM, Paul Sandoz wrote: Hi Joe, It looks a good start. Thanks for the detailed review! There is duplication in the 6 factory finding classes, can some of it be consolidated in one shared factory helper class? The duplication of FactoryFinder and SecuritySupport classes

Re: Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException when using a StringBuffer from multiple threads

2012-06-21 Thread David Schlosnagle
On Thu, Jun 21, 2012 at 11:59 AM, Jim Gish wrote: > + * across threads, must ensure that StringBuffer.add/insert has a Jim, You may want to replace "add" with "append" if you are intending to reference the actual method name and not the generic operation. Additionally, you may want to use {@code

Re: Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException when using a StringBuffer from multiple threads

2012-06-21 Thread Doug Lea
On 06/21/12 11:59, Jim Gish wrote: Please review the proposed spec change to StringBuffer below, which informs the user about additional thread safety issues as described in the bug. (Thanks to Brian for the language). Brian: Nice wording! It misses mentioning the constructor StringBuffer(CharS

Re: Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException when using a StringBuffer from multiple threads

2012-06-21 Thread Ulf Zibis
I would understand, what is meant. Looks good to me. -Ulf Am 21.06.2012 17:59, schrieb Jim Gish: Please review the proposed spec change to StringBuffer below, which informs the user about additional thread safety issues as described in the bug. (Thanks to Brian for the language). Thanks, J

Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException when using a StringBuffer from multiple threads

2012-06-21 Thread Jim Gish
Please review the proposed spec change to StringBuffer below, which informs the user about additional thread safety issues as described in the bug. (Thanks to Brian for the language). Thanks, Jim diff -r 46ff1b63b0c3 src/share/classes/java/lang/StringBuffer.java --- a/src/share/classes/java

Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader

2012-06-21 Thread Paul Sandoz
On Jun 21, 2012, at 2:44 PM, Alan Bateman wrote: > On 21/06/2012 08:55, Joe Wang wrote: >> : >> webrev: >> http://cr.openjdk.java.net/~joehw/jdk8/7169894/webrev/ > It's great to get this work started. > > The javadoc changes look okay to me and fine for this change. However there > are a few ar

Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader

2012-06-21 Thread Alan Bateman
On 21/06/2012 08:55, Joe Wang wrote: : webrev: http://cr.openjdk.java.net/~joehw/jdk8/7169894/webrev/ It's great to get this work started. The javadoc changes look okay to me and fine for this change. However there are a few areas that could be made clearer, like specifying the behavior for t

Re: Patch review request - Test bug 7123972 test/java/lang/annotation/loaderLeak/Main.java fails intermittently

2012-06-21 Thread David Holmes
Hi Eric, On 21/06/2012 8:57 PM, Eric Wang wrote: Hi David, Thanks for your review, I have updated the code by following your suggestion. please see the attachment. I'm not sure whether there's a better way to guarantee object finalized by GC definitely within the given time. The proposed fix ma

Re: Patch review request - Test bug 7123972 test/java/lang/annotation/loaderLeak/Main.java fails intermittently

2012-06-21 Thread Eric Wang
Hi David, Thanks for your review, I have updated the code by following your suggestion. please see the attachment. I'm not sure whether there's a better way to guarantee object finalized by GC definitely within the given time. The proposed fix may work in most cases but may still throw Interru

Re: Patch review request - Test bug 7123972 test/java/lang/annotation/loaderLeak/Main.java fails intermittently

2012-06-21 Thread Alan Bateman
On 21/06/2012 07:05, Eric Wang wrote: Hi All, I come from Java SQE team who are interested in regression test bug fix. Here is the first simple fix for bug 7123972 , Can you please help to review and comment? Attachment is the patch Thanks!

Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader

2012-06-21 Thread Paul Sandoz
Hi Joe, It looks a good start. There is duplication in the 6 factory finding classes, can some of it be consolidated in one shared factory helper class? Taking javax.xml.datatyoe.FactoryFinder as an example: When iterating over the service instances you are catching ConfigurationError 264

Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader

2012-06-21 Thread Joe Wang
jaxp tck passed. Joe On 6/21/2012 12:55 AM, Joe Wang wrote: Hi, This is a patch to replace the manual process in the 3rd step of the JAXP implementation resolution mechanism with ServiceLoader. Please see http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7169894 for more details about the

RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader

2012-06-21 Thread Joe Wang
Hi, This is a patch to replace the manual process in the 3rd step of the JAXP implementation resolution mechanism with ServiceLoader. Please see http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7169894 for more details about the issue. Note that FactoryFinder is duplicated for each JAXP s