RFR: 8226297: Dual-pivot quicksort improvements

Brent Christian brent.christian at oracle.com
Mon Oct 7 22:51:11 UTC 2019


Hi, Vladimir

I've read through the changes[1].  I have the following comments:

---
src/java.base/share/classes/java/util/Arrays.java

* Regarding the indentation changes in the equals() methods:

-     * <pre>    {@code new Double(d1).equals(new Double(d2))}</pre>
+     * <pre>{@code new Double(d1).equals(new Double(d2))}</pre>
...
-     * <pre>    {@code new Double(d1).equals(new Double(d2))}</pre>
+     * <pre>{@code new Double(d1).equals(new Double(d2))}</pre>
...
-     * <pre>    {@code new Float(f1).equals(new Float(f2))}</pre>
+     * <pre>{@code new Float(f1).equals(new Float(f2))}</pre>
...
-     * <pre>    {@code new Float(f1).equals(new Float(f2))}</pre>
+     * <pre>{@code new Float(f1).equals(new Float(f2))}</pre>

Looking at the generated JavaDoc, this indentation is intentional.  I've 
reverted these changes.

* Otherwise, the JavaDoc changes are limited to within @implNote 
(previously written out as "Implementation note", in some cases).  I 
don't believe a CSR is warranted.

---
test/jdk/java/util/Arrays/Sorting.java

Some general comments:

* I see a lot of renames[2] that I don't think add value, and clutter 
the changes (1000+ total changed lines in a 2000 line file).  I feel 
similarly about the changes from post- to pre- incrementing of the for() 
loop indices.  If it's OK with you, Vladimir, I can take care of 
reverting these changes.

* I like that the the various DualPivotQuickSort methods are tested 
directly (via SortingHelper).  I'd prefer that we also continue testing 
direct calls to Arrays.sort(), as that's what users will call.  What 
would be the best way to add that back in?  (Maybe a SortingHelper 
variant that calls Arrays.sort()?).

* Having the test operation depend on setting various values to the 
static 'sortingHelper' field seems brittle to me.  This could be a good 
opportunity to convert from static to instance methods, make 
'sortingHelper' and 'name' member variables, and create separate Sorting 
objects for each SortingHelper. Such a refactoring is not strictly 
necessary, but I think it would be nice to have.

---
test/jdk/java/util/Arrays/FailedFloat.java

Is this test necessary, given the addition of testFloatNegativeZero() in 
Sorting.java ?

Thanks,
-Brent

1. 
http://cr.openjdk.java.net/~bchristi/8226297/webrev05-adaptive/webrev/index.html

2. Name changes in Sorting.java:
     testAndCheck -> testQuicksort
     seed -> random
     ourDescription -> name
     failedSort -> failSort
     failed -> fail
     myKey -> key
     myValue -> value
     Merge->Merging [Sort|Builder]
     MyRandom->TestRandom
     failedCompare->failCompare
     change TypeConverter, FloatBuilder, DoubleBuilder, SortedBuilder to 
be non-static



More information about the core-libs-dev mailing list