Re: [PATCH 4/4] bb-reorder: convert to gcc_stablesort

[Michael Matz <matz at suse dot de>]

Hi,

On Tue, 28 Aug 2018, Alexander Monakov wrote:

> > I think your proposed one
> > warrants some comments.  Maybe trade speed for some clearer code?
> 
> I think it's not too complicated, but how about adding this comment:
> 
>   profile_count m = c1.max (c2);
>   /* Return 0 if counts are equal, -1 if E1 has the larger count.  */
>   return (m == c2) - (m == c1);
> 
> Or, alternatively, employ more branchy checks:
> 
>   if (c1 == c2)
>     return 0;
>   if (c1 == c1.max (c2))
>     return -1;
>   return 1;

You want to at least comment on why a tradition </>/== check isn't 
working, namely because of the uninitialized counts.  (Though I also think 
that comparing uninitialized counts is just non-sense and hence should 
actually be ruled out from the whole comparison business, at which point a 
normal <= check can be done again).

> > The comments on the operator<> implementations are odd as well:
> 
> (an aside, but: it also has strange redundant code like
> 
>       if (*this  == profile_count::zero ())
>         return false;
>       if (other == profile_count::zero ())
>         return !(*this == profile_count::zero ());
> 
> where the second return could have been 'return true;')

Oh my.


Ciao,
Michael.