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

[Rémi Forax]

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.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
>