Review request for 8001536

[Remi Forax]

On 10/30/2012 07:53 PM, Lance Andersen - Oracle wrote:
> Hi Remi,
>
>
> Thank you for the feedback
>
> 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.
> OK, thanks for the suggestion, I will make this change
>> 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;
> I can simplify hashCode to the what you have above, I liked the convenience method which is why I was using it. But happy to change it

In fact, thinking a little more about that, Objects.hash() even don't 
provide the semantics you want
because it calls Arrays.hashCode() and not Arrays.deepEquals() so 
Objects.hash(buf, ..., ....) calls
buf.hashCode() and not Arrays.hashCode(buf).

>> in clone, sb should not be initialized to null
> I think it is OK as I have it as  HashMap does it similar to what I have done vs ArrayList which follows your suggestion.  Do we have a preferred practice or is this just a style choice?
>> and the catch should be: throw new InternalError(e),
> Given I am providing clone(), I did not see a reason to provide InternalError().  I have no strong preference but some java classes do and others do not (HashMap for example), so what is our preferred standard?

I don't think it's a good idea to let the catch empty and I don't like 
to have to initialize a variable
in the code for something that never occurs.
Maybe something like:

A a;
try {
   a = clone();
} catch(CloneNotSupportedException e) {
   throw null;  // should never be executed
}
...

throw null will not produce a big bytecode unlike new InternalError(e)
and because it's a throw, the compiler will not require to initialize 'a'.

And this pattern should be enforced in the whole JDK.

>> this is the standard code you can see in clone.
>>
>> in readObject, the test (buf.length != len) can be done before decoding the blob.
> True, I can move it up.
>> in writeObject, you set "blob" twice, which is weird,
> my bad, I forgot to remove that.
>> 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.
> This is intentional.  A Blob or Clob will not be serializable as its properties are unique to the database and it is created from an active Connection object.
>
> In the event someone actually tried to serialize this, I do not want it to fail just because someone passed in a LOB to instantiate this beast (note these methods should never have been created this way but this is way before my time).
>
> In the unlikely event someone created their own wrapped Blob/Clob (which I cannot see why anyone would do), I am allowing for both for backwards compatibility.

Ok, maybe you should add a comment.

>
> Best
> Lance

kind regards,
Rémi