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
