Re: Review request for 8001536 updated
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
On 10/31/2012 04:08 PM, Lance Andersen - Oracle wrote: Here is revised webrev taking into account Remi's suggestions http://cr.openjdk.java.net/~lancea/8001536/webrev.01/ looks good :) Best, Lance Rémi 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
Re: Review request for 8001536 updated
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
Re: Review request for 8001536
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
Re: Review request for 8001536
On 30/10/2012 18:05, Remi Forax wrote: 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. Yes, that is odd. I think the issue here is that serialized form should never have included the blob in the first place. -Alan
Re: Review request for 8001536
Point taken Ulf, thank you for the feedback and will follow your suggestion going forward , which I typically do, but I think I am still feeling the effects of being offline due to hurricane sandy :-( Best Lance On Oct 30, 2012, at 2:50 PM, Ulf Zibis wrote: > Thanks Lance. > > But having the subject of the request in clear text in the list view of the > email client would be a great help. > > -Ulf > > > Am 30.10.2012 19:28, schrieb Lance Andersen - Oracle: >> Hi Ulf, >> >> The bug is described below, it is just adding the read/writeObject and clone >> methods. >> >> Best >> Lance >> On Oct 30, 2012, at 2:18 PM, Ulf Zibis wrote: >> >>> Hi all, >>> >>> please add the bug summary to the subject line. >>> Bug 8001536 is not publicly visible :-( >>> >>> -Ulf >>> >>> Am 30.10.2012 17:25, schrieb Lance Andersen - Oracle: 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 . 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 >> > Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: Review request for 8001536
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 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? > 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. Best Lance > > 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
Re: Review request for 8001536
Thanks Lance. But having the subject of the request in clear text in the list view of the email client would be a great help. -Ulf Am 30.10.2012 19:28, schrieb Lance Andersen - Oracle: Hi Ulf, The bug is described below, it is just adding the read/writeObject and clone methods. Best Lance On Oct 30, 2012, at 2:18 PM, Ulf Zibis wrote: Hi all, please add the bug summary to the subject line. Bug 8001536 is not publicly visible :-( -Ulf Am 30.10.2012 17:25, schrieb Lance Andersen - Oracle: 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 . 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
Re: Review request for 8001536
Hi Ulf, The bug is described below, it is just adding the read/writeObject and clone methods. Best Lance On Oct 30, 2012, at 2:18 PM, Ulf Zibis wrote: > Hi all, > > please add the bug summary to the subject line. > Bug 8001536 is not publicly visible :-( > > -Ulf > > Am 30.10.2012 17:25, schrieb Lance Andersen - Oracle: >> 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 . >> >> >> 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
Re: Review request for 8001536
Hi all, please add the bug summary to the subject line. Bug 8001536 is not publicly visible :-( -Ulf Am 30.10.2012 17:25, schrieb Lance Andersen - Oracle: 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 . 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
Re: Review request for 8001536
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
Review request for 8001536
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 . 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
