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

[Lance Andersen - Oracle]

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


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