Compiler warnings in __Pyx_PyInt_TrueDivideObjC with -Wconversion

[robertwb]

Reported by nikratio on 11 Sep 2015 22:58 UTC The code generated by Cython 0.23.1 for the __Pyx_PyInt_TrueDivideObjC triggers the following compiler warning:

src/llfuse.c: In function __Pyx_PyInt_TrueDivideObjC:
src/llfuse.c:40913:52: error: conversion to int from Py_ssize_t may alter its value [-Werror=conversion]
                 if (8 * sizeof(long) <= 53 || (abs(size) <= 52 / PyLong_SHIFT) || likely(labs(a) <= (1L << 53))) {
                                                    ^

It'd be great if Cython generated C would compile without warnings.

I tried to produce a smaller example, but I failed to find the code that's responsible for the generation of this function.

The full source that triggers the problem is available at https://github.com/python-llfuse/main - run ./setup.py build_cython to generate src/llfuse.c.

Migrated-From: http://trac.cython.org/ticket/863

[robertwb]

Modified by scoder on 12 Sep 2015 13:45 UTC

[robertwb]

Comment by nikratio on 12 Sep 2015 18:32 UTC This is actually a regression, there are no warnings with Cython 0.21.1.

[robertwb]

Comment by scoder on 12 Sep 2015 18:53 UTC The function is implemented in Cython/Utility/Optimize.c. This is new code in 0.23.

[robertwb]

Comment by nikratio on 12 Sep 2015 18:53 UTC This got introduced in the following commit:

changeset:   10320:d24e67add5d9
git commit:  7008e3a9c36a49dbc2a13a4999c11a376d8b2b27
user:        Stefan Behnel <stefan_ml@behnel.de>
date:        Wed Apr 01 21:21:54 2015 +0200
files:       Cython/Utility/Optimize.c
description:
use abs() instead of two min-max value comparisons

--- a/Cython/Utility/Optimize.c
+++ b/Cython/Utility/Optimize.c
@@ -544,7 +544,7 @@
             x += ((x != 0) & ((x ^ b) < 0)) * b;
             return PyInt_FromLong(x);
         {{elif op == 'TrueDivide'}}
-            if (8 * sizeof(long) <= 53 || likely({{ival}} <= (1L << 53) && {{ival}} >= (-(1L << 53)))) {
+            if (8 * sizeof(long) <= 53 || likely(labs({{ival}}) <= (1L << 53))) {
                 return PyFloat_FromDouble((double)a / (double)b);
             }
             // let Python do the rounding
@@ -626,7 +626,7 @@
                 x = a % b;
                 x += ((x != 0) & ((x ^ b) < 0)) * b;
             {{elif op == 'TrueDivide'}}
-                if (8 * sizeof(long) <= 53 || (size >= -52 / PyLong_SHIFT && size <= 52 / PyLong_SHIFT) || likely({{ival}} <= (1L << 53) && {{ival}} >= (-(1L << 53)))) {
+                if (8 * sizeof(long) <= 53 || (abs(size) <= 52 / PyLong_SHIFT) || likely(labs({{ival}}) <= (1L << 53))) {
                     return PyFloat_FromDouble((double)a / (double)b);
                 }
                 return PyLong_Type.tp_as_number->nb_{{slot_name}}(op1, op2);

[robertwb]

Comment by nikratio on 12 Sep 2015 18:56 UTC If we want to use the abs function here, we need to add another case distinction based on sizeof(ssize_t) and call either abs, labs or llabs. That could still be evaluated at compile time, but is the performance gain from using abs instead of the two comparisons really significant?

[robertwb]

Comment by scoder on 12 Sep 2015 19:16 UTC Thanks, here's a fix:

https://github.com/cython/cython/commit/c7227ac781764ac643d3d0e64eed0cf1079e5cea

[robertwb]

Modified by scoder on 12 Sep 2015 19:16 UTC

[robertwb]

Modified by scoder on 12 Sep 2015 20:45 UTC