[Spice-devel] [PATCH spice-gtk] main: Avoid sqrt in monitor compare

[Frediano Ziglio]

> 
> > On 24 Feb 2017, at 13:26, Pavel Grunt <pgrunt at redhat.com> wrote:
> > 
> > qsort cares about the sign of the difference and that should stay
> > the same
> > ---
> > src/channel-main.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index cd3dee7..50c29f2 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -17,7 +17,6 @@
> > */
> > #include "config.h"
> > 
> > -#include <math.h>
> > #include <spice/vd_agent.h>
> > #include <glib/gstdio.h>
> > #include <glib/gi18n-lib.h>
> > @@ -1029,8 +1028,8 @@ static int monitors_cmp(const void *p1, const void
> > *p2, gpointer user_data)
> > {
> >   const VDAgentMonConfig *m1 = p1;
> >   const VDAgentMonConfig *m2 = p2;
> > -    double d1 = sqrt(m1->x * m1->x + m1->y * m1->y);
> > -    double d2 = sqrt(m2->x * m2->x + m2->y * m2->y);
> > +    double d1 = m1->x * m1->x + m1->y * m1->y;
> > +    double d2 = m2->x * m2->x + m2->y * m2->y;
> 
> If there really are chances for any of the coordinates to go beyond 16-bit, I
> would write this as:
> 
> 	long m1x = m1->x;
> 	long m1y = m1->y;
> 	long m2x = m2->x;
> 	long m2y = m2->y;
> 	long d1 = m1x * m1x + m1y * m1y;
> 	long d2 = m2x * m2x + m2y * m2y;
> 
> The extra cost is zero on x86_64, and it’s giving you the correct result at a
> lower cost than double.
> 

long d1 = (long)m1->x * m1->x + (long)m1->y * m1->y;

is fine too.

> >   int diff = d1 - d2;
> > 

This would overflow potentially.

> >   return diff == 0 ? (char*)p1 - (char*)p2 : diff;
> 
> If we only care about the sign of the difference, I would rewrite this as:
> 
> 	return diff != 0 ? diff : (p1 > p2) - (p1 < p2);
> 
> The second expression is the sign of the difference between the two pointers,
> but it’s guaranteed to return something good even if you subtract 64-bit
> pointers with 32-bit int, as on x86-64. In that case, with the current code,
> you could be unlucky and have two pointers that are more than 2G apart and
> get a wrong result.
> 

The function is called for sorting an array of monitors, I hope the pointers
are not so far away, if it happens we'd have other issues.

Frediano