RFR: 8226297: Dual-pivot quicksort improvements

[Brent Christian]

Hi, Vladimir

On 10/8/19 3:18 PM, Vladimir Yaroslavskiy wrote:
> 
> The following renaming changes are important
> and should be kept. My explanation is here
 > ...

Thanks for the explanation - much appreciated.

> * Other changes, like failed -> fail, i++ -> ++i,
> and TypeConverter, FloatBuilder, DoubleBuilder, SortedBuilder
> to be non-static can be reverted.
> 
> I can revert them, if you agree.

That would be great, thanks.

>     * I like that 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()?).
> 
> I will add direct calls to Arrays.sort() and Arrays.parallelSort() also.

Great, thank you.

>     * 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.
> 
> 
> Good idea, I will implement it and send you update version very soon.

Sounds good!

>>     test/jdk/java/util/Arrays/FailedFloat.java
>> 
>>     Is this test necessary, given the addition of
>>     testFloatNegativeZero() in
>>     Sorting.java ?
> 
> Not necessary, because testFloatNegativeZero() covers this case.
> This class was created to reproduce bug when float-pointing zeros
> and NaNs are sorted.
> 
> Please remove FailedFloat.java. It makes sense to add the source of this 
> class to 8226297.

Done and done.

-Brent