Review needed for 7132879 to address the findbug errors in CachedRowSetImpl, SerialStruct, BaseRow, SerialInputImpl, SerialOutputImpl

[Rémi Forax]

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.Andersen at oracle.com
>