Compiler warnings in __Pyx_PyInt_TrueDivideObjC with -Wconversion

[robertwb]

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]

scoder changed description from

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

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

[robertwb]

@Nikratio changed This is actually a regression, there are no warnings with Cython 0.21.1.

[robertwb]

scoder changed The function is implemented in Cython/Utility/Optimize.c. This is new code in 0.23.

[robertwb]

@Nikratio changed 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]

@Nikratio changed 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]

scoder changed owner from somebody to nikratio Thanks, here's a fix:

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

[robertwb]

scoder changed resolution to fixed status from new to closed

[robertwb]

scoder changed milestone from wishlist to 0.23.3