RFR: 8226297: Dual-pivot quicksort improvements
Brent Christian
brent.christian at oracle.com
Wed Oct 9 00:41:28 UTC 2019
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
More information about the core-libs-dev
mailing list