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