RFR: 8185757: QuickSort array size should be size_t

[Thomas Schatzl]

Hi,

On Thu, 2017-08-03 at 14:06 -0400, Kim Barrett wrote:
> > 
> > On Aug 3, 2017, at 8:32 AM, Thomas Schatzl <thomas.schatzl at oracle.c
> > om> 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.

Okay.

> > 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.

Actually I was already at writing about an issue with indentation when
I noticed the brackets :)

Not insisting on changing this.

> > > 
> > > 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.

Just commenting when I read over it. Yes, multiple tests running at the
same time another aspect.

Thanks,
  Thomas