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

Lance Andersen - Oracle Lance.Andersen at oracle.com
Fri Jan 27 00:03:37 UTC 2012

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, aload1 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

More information about the core-libs-dev mailing list