Re: PATCH [6901992] : Possible InvalidJarIndexException due to bug in sun.misc.JarIndex.merge()

2012-06-13 Thread Diego Belfer
Hi Chris, There is no way to generate a jar without directory entries using the jar tool; there is no option for that. What do you think is the best way to handle this ? I don't want to re-implement the logic for creating a jar using the JarOutputStream class. Do you think it is possible to add a

Re: PATCH [6901992] : Possible InvalidJarIndexException due to bug in sun.misc.JarIndex.merge()

2012-06-13 Thread Diego Belfer
Chris, I was thinking of something similar. I will create the jars and their contents using Java code. There is no need of creating real class files, using a few resource files and some directories will be enough. Best, Diego On Wed, Jun 13, 2012 at 6:46 PM, chris hegarty wrote: > Diego, > > I'

Re: Review request for 7145913 CachedRowSetSwriter.insertNewRow() throws SQLException

2012-06-13 Thread Ulf Zibis
Am 13.06.2012 13:04, schrieb Lance Andersen - Oracle: Hi Paul, Thank you for taking the time to review the code. I made the change you suggested below http://cr.openjdk.java.net/~lancea/7145913/webrev.02 Typos: 912 * Hence we cannot exactly identify why the insertion faile

Re: PATCH [6901992] : Possible InvalidJarIndexException due to bug in sun.misc.JarIndex.merge()

2012-06-13 Thread chris hegarty
Diego, I'm thinking that some of the trivial source files, to compile and built into the jars, could be simply created and written by the test itself, rather than checking them all in. If this makes it cleaner. I really don't like all the file in test/sun/misc/JarIndex/metaInfFilenames, but a

Re: PATCH [6901992] : Possible InvalidJarIndexException due to bug in sun.misc.JarIndex.merge()

2012-06-13 Thread Diego Belfer
Hi Chris, That's right. The only non-cleanup change is the one in the merge. Regarding the test case, I will re-write them in order to generate the jars on fly. I'd scanned the jdk/test folder and found a few jars, that's why I included them. I have seen your test case, I will use it as a sampl

Re: Review request for 7145913 CachedRowSetSwriter.insertNewRow() throws SQLException

2012-06-13 Thread Rémi Forax
On 06/13/2012 07:01 PM, Lance Andersen - Oracle wrote: Hi Remi, Thank you for the suggestion. Over the years, I have gotten different views on whether to have multiple return points vs just one. Is there a specific style preference that should be used going forward? At this time, I would

Re: Review request for 7145913 CachedRowSetSwriter.insertNewRow() throws SQLException

2012-06-13 Thread Lance Andersen - Oracle
Hi Remi, Thank you for the suggestion. Over the years, I have gotten different views on whether to have multiple return points vs just one. Is there a specific style preference that should be used going forward? At this time, I would prefer to not make another change and if the consensus g

Re: Review request for 7145913 CachedRowSetSwriter.insertNewRow() throws SQLException

2012-06-13 Thread Rémi Forax
On 06/13/2012 06:18 PM, Joe Wang wrote: Hi Lance, The changes look good to me. Joe Hi Lance, just a minor comment, in isPKNameValid, you don't need the boolean isValid because you can return true instead of using break and return false at the end. cheers, Rémi On 6/13/2012 4:09 AM, Paul

Re: Review request for 7145913 CachedRowSetSwriter.insertNewRow() throws SQLException

2012-06-13 Thread Joe Wang
Hi Lance, The changes look good to me. Joe On 6/13/2012 4:09 AM, Paul Sandoz wrote: On Jun 13, 2012, at 1:04 PM, Lance Andersen - Oracle wrote: Hi Paul, Thank you for taking the time to review the code. I made the change you suggested below http://cr.openjdk.java.net/~lancea/7145913/webr

Re: PATCH [6901992] : Possible InvalidJarIndexException due to bug in sun.misc.JarIndex.merge()

2012-06-13 Thread Chris Hegarty
Hi Diego, Thanks for picking up this bug. I think your changes look fine. Mainly cleanup except for add -> addExplicit/addMapping in merge, right? BTW the cleanup makes this more readable. Unfortunately, the tests you created require checking in a binary jar file. This is a real no no for t

hg: jdk8/tl/jdk: 7176574: sun/security/krb5/auto/TcpTimeout.java failed with solaris-i586

2012-06-13 Thread weijun . wang
Changeset: 4435f8b20d08 Author:weijun Date: 2012-06-13 19:23 +0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4435f8b20d08 7176574: sun/security/krb5/auto/TcpTimeout.java failed with solaris-i586 Reviewed-by: chegar ! test/sun/security/krb5/auto/TcpTimeout.java

Re: Review request for 7145913 CachedRowSetSwriter.insertNewRow() throws SQLException

2012-06-13 Thread Paul Sandoz
On Jun 13, 2012, at 1:04 PM, Lance Andersen - Oracle wrote: > Hi Paul, > > Thank you for taking the time to review the code. > > > I made the change you suggested below > > http://cr.openjdk.java.net/~lancea/7145913/webrev.02 > > Let me know if you are good with the change and I will get this

Re: Review request for 7145913 CachedRowSetSwriter.insertNewRow() throws SQLException

2012-06-13 Thread Lance Andersen - Oracle
Hi Paul, Thank you for taking the time to review the code. I made the change you suggested below http://cr.openjdk.java.net/~lancea/7145913/webrev.02 Let me know if you are good with the change and I will get this puppy put back. Best Lance On Jun 13, 2012, at 3:53 AM, Paul Sandoz wrote: >

hg: jdk8/tl/jdk: 2 new changesets

2012-06-13 Thread kelly . ohair
Changeset: 9fd127ff51d5 Author:ohair Date: 2012-06-12 13:54 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/9fd127ff51d5 7176138: Fixes for missing close() calls and possible null pointer reference instead of fatal error Reviewed-by: dcubed ! src/share/demo/jvmti/hprof/hprof

Re: Review request CR 7171918 XmlReaderContentHandler.endElement does not handle a Delete Tag properly

2012-06-13 Thread Paul Sandoz
On Jun 12, 2012, at 11:11 PM, Joe Wang wrote: > Looks good to me, Lance. > +1 Paul.

Re: Review request for 7145913 CachedRowSetSwriter.insertNewRow() throws SQLException

2012-06-13 Thread Paul Sandoz
Hi Lance, It looks OK to me, however i don't know much about JDBC. Much cleaner than before. Very minor point, you can shoot me for being pedantic!: from line 894 you can do the following since the return value from pstmt.executeUpdate() is never used: 894try { 895