Hi,
I've created a webrev of the latest test changes that Vladimir sent me:
http://cr.openjdk.java.net/~bchristi/8226297/webrev09-testUpdate/
(I also created an incremental webrev[1] along the way, in case that's
helpful to anyone).
I have just a few comments:
test/jdk/java/util/Arrays/Sorting.java
* I found some instances of "FloatPointing", which I've already changed
to "FloatingPoint". :)
--
// Array lengths used in a long run (default)
private static final int[] LONG_RUN_LENGTHS = {
- 1, 2, 3, 5, 8, 13, 21, 34, 55, 100, 1000, 10000, 100000, 1000000 };
+ 1, 3, 8, 21, 55, 100, 1_000, 10_000, 100_000 };
// Array lengths used in a short run
private static final int[] SHORT_RUN_LENGTHS = {
- 1, 2, 3, 21, 55, 1000, 10000 };
+ 1, 8, 55, 100, 10_000 };
Vladimir, can you tell me why some of the length values have been removed?
--
1570 private static enum TypeConverter {
...
1637 abstract void convert(Object src, Object dst);
The 'src' argument to convert is always cast to an int[]. Can it be
made an int[] in the method signature, instead of Object ? I can make
the change if that sounds good.
Other than those, this is pretty much ready to go.
Thanks,
-Brent
1. http://cr.openjdk.java.net/~bchristi/8226297/webrev09-incremental/
On 10/8/19 5:41 PM, Brent Christian wrote:
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
