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

Reply via email to