>
> > On 24 Feb 2017, at 13:26, Pavel Grunt <pgrunt@xxxxxxxxxx> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel