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
