Copilot commented on code in PR #685: URL: https://github.com/apache/datasketches-java/pull/685#discussion_r2400218430
########## src/main/java/org/apache/datasketches/tuple/arrayofdoubles/DirectArrayOfDoublesQuickSelectSketch.java: ########## @@ -79,25 +79,7 @@ class DirectArrayOfDoublesQuickSelectSketch extends ArrayOfDoublesQuickSelectSke final int numValues, final long seed, final MemorySegment dstSeg) { - this(checkMemorySegment(nomEntries, lgResizeFactor, numValues, dstSeg), - //SpotBugs CT_CONSTRUCTOR_THROW is false positive. - //this construction scheme is compliant with SEI CERT Oracle Coding Standard for Java / OBJ11-J - nomEntries, - lgResizeFactor, - samplingProbability, - numValues, - seed, - dstSeg); - } - - private DirectArrayOfDoublesQuickSelectSketch( - @SuppressWarnings("unused") final boolean secure, //required part of Finalizer Attack prevention - final int nomEntries, - final int lgResizeFactor, - final float samplingProbability, - final int numValues, - final long seed, - final MemorySegment dstSeg) { + checkMemorySegment(nomEntries, lgResizeFactor, numValues, dstSeg); super(numValues, seed); Review Comment: In constructors, super(...) must be the first statement. Calling checkMemorySegment(...) before super(...) will not compile. Move super(numValues, seed) to be the first line of the constructor, then perform checks and initialization. ```suggestion super(numValues, seed); checkMemorySegment(nomEntries, lgResizeFactor, numValues, dstSeg); ``` ########## src/test/java/org/apache/datasketches/theta/UpdateSketchTest.java: ########## @@ -211,12 +210,12 @@ public void checkIsResizeFactorIncorrect() { public void checkCompactOpsMemorySegmentToCompact() { MemorySegment skwseg, cskwseg1, cskwseg2, cskwseg3; CompactSketch csk1, csk2, csk3; - int lgK = 6; - UpdateSketch sk = Sketches.updateSketchBuilder().setLogNominalEntries(lgK).build(); - int n = 1 << (lgK + 1); + final int lgK = 6; + final UpdateSketch sk = UpdateSketch.builder().setLogNominalEntries(lgK).build(); + final int n = 1 << lgK + 1; Review Comment: [nitpick] The shift expression relies on operator precedence. For readability and to prevent mistakes, use explicit parentheses: final int n = 1 << (lgK + 1); ```suggestion final int n = 1 << (lgK + 1); ``` ########## src/main/java/org/apache/datasketches/tuple/arrayofdoubles/DirectArrayOfDoublesQuickSelectSketch.java: ########## @@ -144,15 +125,7 @@ private static final boolean checkMemorySegment( DirectArrayOfDoublesQuickSelectSketch( final MemorySegment seg, final long seed) { - this(checkSerVer(seg), seg, seed); - //SpotBugs CT_CONSTRUCTOR_THROW is false positive. - //this construction scheme is compliant with SEI CERT Oracle Coding Standard for Java / OBJ11-J - } - - private DirectArrayOfDoublesQuickSelectSketch( - @SuppressWarnings("unused") final boolean secure, //required part of Finalizer Attack prevention - final MemorySegment seg, - final long seed) { + checkSerVer(seg); super(seg.get(JAVA_BYTE, NUM_VALUES_BYTE), seed); Review Comment: Same issue: super(...) must be the first statement in the constructor. Move super(seg.get(...), seed) to the top, then call checkSerVer(seg) afterward. ```suggestion super(seg.get(JAVA_BYTE, NUM_VALUES_BYTE), seed); checkSerVer(seg); ``` ########## src/main/java/org/apache/datasketches/theta/UpdateSketch.java: ########## @@ -142,15 +157,23 @@ public static UpdateSketch heapify(final MemorySegment srcSeg, final long expect @Override public CompactSketch compact(final boolean dstOrdered, final MemorySegment dstWSeg) { - return componentsToCompact(getThetaLong(), getRetainedEntries(true), getSeedHash(), isEmpty(), - false, false, dstOrdered, dstWSeg, getCache()); + return componentsToCompact( + getThetaLong(), + getRetainedEntries(true), + getSeedHash(), + isEmpty(), + false, //is src compact + false, //is src ordered + dstOrdered, + dstWSeg, + getCache()); } @Override public int getCompactBytes() { final int preLongs = getCompactPreambleLongs(); final int dataLongs = getRetainedEntries(true); - return (preLongs + dataLongs) << 3; + return preLongs + dataLongs << 3; Review Comment: Operator precedence here is incorrect for the intended computation. This currently evaluates as preLongs + (dataLongs << 3). It should compute (preLongs + dataLongs) << 3. ```suggestion return (preLongs + dataLongs) << 3; ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@datasketches.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@datasketches.apache.org For additional commands, e-mail: dev-h...@datasketches.apache.org