On 01/27/2012 01:03 AM, Lance Andersen - Oracle wrote:
Hi Remi,
thanks for the additional info. Made the extra
changehttp://cr.openjdk.java.net/~lancea/7133815/webrev.02, so I hope we are
good to go now.
Best
Lance
thumb up :)
Rémi
On Jan 26, 2012, at 6:47 PM, Rémi Forax wrote:
On 01/27/2012 12:12 AM, Lance Andersen - Oracle wrote:
Hi Remi,
Thanks for the feedback, I made the suggested changes
http://cr.openjdk.java.net/~lancea/7133815/webrev.01/ and re-ran the rowset tck
.
Best
Lance
Lance,
sorry to not have been crystal clear in my previous email,
I've forgotten to mention that because the field is stored in a local variable
to do the nullcheck you can reuse the same local variable instead of
reloading the value from the field which is a good code practice.
So instead of
int[] keyColumns = this.keyCols;
return (keyColumns == null) ? null : Arrays.copyOf(keyCols, keyCols.length);
the code should be
int[] keyColumns = this.keyCols;
return (keyColumns == null) ? null :
Arrays.copyOf(keyColumns,keyColumns.length);
Now, this is not strictly needed for the codes of the changeset because the JIT
can easily prove in these codes that the field will not be changed between
the first load and the subsequent accesses as arguments of Arrays.copyOf.
Anyway, as i said, it's a good practice to avoid to do several getfields
if the value is not supposed to change in between.
Rémi
PS: as a bonus, using local variables reduces the size of the generated
bytecodes,
aload_1 takes one byte and getfield takes three bytes :)
On Jan 26, 2012, at 5:21 PM, Rémi Forax wrote:
On 01/26/2012 10:46 PM, Lance Andersen - Oracle wrote:
Hi,
Looking for a reviewer for http://cr.openjdk.java.net/~lancea/7133815/webrev.00/
this is a small change to address 2 findbug errors. The findbug issues being
addressed:
May expose internal representation by returning reference to mutable object
and
Load of known null value
For CachedRowSetImpl, SerialStruct, BaseRow, SerialInputImpl, SerialOutputImpl
Hi Lance,
I think it's better to use array.clone() or
Arrays.copyOf(array, array.length) which is a little faster.
so the code of CachedRowSetImpl should be:
int[]keyCols = this.keyCols;
return (keyCols == null)? null: Arrays.copyOf(keyCols, keyCols.length);
otherwise the code of SQLOutputImpl is ok for me.
cheers,
Rémi
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
