Re: Review request for 8001536 updated

[Alan Bateman]

On 31/10/2012 15:08, Lance Andersen - Oracle wrote:

Here is revised webrev taking into account Remi's suggestions 
http://cr.openjdk.java.net/~lancea/8001536/webrev.01/



I skimmed over the updated webrev and the changes mostly look okay to me.

One comment on the clone method is that The internal {@code Blob} field 
will be set to null doesn't seem right. Shouldn't this say that the 
resulting object doesn't have an underlying Blob?


I don't know if you want formatting/type comments but a couple of nits:
- In both classes then it looks like the javadoc comment on equals has 
been shunted right by space
- In equals then if( seems to be missing a space between if and (. 
There's another one in readObject.
- The spacing around + in hashCode is a bit odd, it doesn't matter of 
course but would be good to be consistent
- the first line of both readObject has also been shunted right by one 
space.


-Alan.

Re: Review request for 8001536 updated

[Lance Andersen - Oracle]

Here is revised webrev taking into account Remi's suggestions 
http://cr.openjdk.java.net/~lancea/8001536/webrev.01/


Best,
Lance


On Oct 30, 2012, at 2:05 PM, Remi Forax wrote:

 On 10/30/2012 05:25 PM, Lance Andersen - Oracle wrote:
 Hi,
 
 This is a request for review of 
 http://cr.openjdk.java.net/~lancea/8001536/webrev.00/.  This adds 
 read/writeObject as well as clone methods to SerialXLob classes.
 
 All SQE tests passed,  1 failure in the RowSet JCK/TCK tests due to a bug in 
 the test that the TCK team is aware of and will address.  JDBC Unit tests 
 all pass .
 
 Hi Lance.
 In SerialBlob and in SerialClob
 test (obj == null) is not necessary in equals, null instanceof X is always 
 false.
 
 in hashCode, Objects.hash() allocate an array to pass arguments to 
 Arrays.hashCode() and box primitive values to Object.
 while this method is really convenient to use, each calls will allocate an 
 array and box the two values,
 the overhead seems to high here.
 This code should be equivalent:
return ((31 +Arrays.hashCode(buf)) * 31 +len) * 31 + origLen;
 
 in clone, sb should not be initialized to null and the catch should be: throw 
 new InternalError(e),
 this is the standard code you can see in clone.
 
 in readObject, the test (buf.length != len) can be done before decoding the 
 blob.
 
 in writeObject, you set blob twice, which is weird, also I think that if 
 blob is not Serializable,
 the code should throw an exception, so you should not use instanceof and let 
 s.writeFields()
 to throw NotSerializable exception.
 
 cheers,
 Rémi
 
 
 
 Best
 Lance
 
 Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
 Oracle Java Engineering
 1 Network Drive
 Burlington, MA 01803
 lance.ander...@oracle.com
 
 


Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com