RFR: 8185757: QuickSort array size should be size_t

[Kim Barrett]

> On Aug 3, 2017, at 8:32 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> On Wed, 2017-08-02 at 19:29 -0400, Kim Barrett wrote:
>> - Changed the handling of the result of calling the comparator, only
>> requiring it to return negative, zero, or positive, rather than
>> exactly -1, 0, or +1. This makes it consistent with the standard
>> library function qsort.
> 
> Not sure if the change of the do-while to the for-loops improves
> readability that much.

I guess it wasn’t obvious that the change away from do-while was
necessitated by the change from int to size_t.  The left/right_index
variables were initialized to beyond the end, and always pre-inc/dec
in the do-while loops.  But in that scheme, left_index was initially -1.

While it would have worked to change the types to size_t and still
initialize left_index to (converted) -1, and left the do-while untouched
so that it incremented left_index to (overflowed) zero in the first
iteration, that seemed rather obscure to me, and also at some risk of
compiler / static analyzer warnings.

> However, please put the closing brackets of these into extra lines
> (quicksort.hpp:76,77) to avoid the casual reader to overlook them.

Sorry, but that just looks horrible.  As a casual reader, I wouldn’t
even look for them, since if they aren’t there then the code is
badly mis-indented.

>> - In Method::sort_methods, removed unnecessary explicit template
>> argument, allowing it to be deduced.  I didn't change the length from
>> int to size_t here, because that had more fanout, and there are other
>> issues around its type.  For example, it is being passed to other
>> functions that expect a u2 value.
> 
> Fine with me. Please consider filing a CR for that.

I was thinking I should probably do that.

>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8185757
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8185757/hotspot.00/
>> JPRT. In addition to unit testing, sorting gets exercised by G1 and
>> by class file parsing (sort_methods).
> 
> One (potential) pre-existing issue with the test: it uses random() to
> create tests - however this decreases reproducability…

Which has both good and bad aspects.  Printing the first random value
might help, so long as this remains a non-TEST_VM test; once the
VM is initialized there could be other concurrent callers of random()
that would alter the test’s sequence.