Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Casting -1 to unsigned C++ enum type generates warning #2906

Open
pitrou opened this Issue Mar 26, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@pitrou
Copy link
Contributor

commented Mar 26, 2019

With gcc and -Wconversion enabled, I get the following kind of warnings with Cython 0.26.1:

/home/antoine/arrow/python/pyarrow/lib.cpp: In function ‘arrow::Type::type __Pyx_PyInt_As_enum____arrow_3a__3a_Type_3a__3a_type(PyObject*)’:
/home/antoine/arrow/python/pyarrow/lib.cpp:147883:43: warning: the result of the conversion is unspecified because ‘-1’ is outside the range of type ‘arrow::Type::type’ [-Wconversion]
                     return (target_type) -1;\
                                           ^
/home/antoine/arrow/python/pyarrow/lib.cpp:147873:5: note: in expansion of macro ‘__PYX__VERIFY_RETURN_INT’
     __PYX__VERIFY_RETURN_INT(target_type, func_type, func_value, 0)
     ^~~~~~~~~~~~~~~~~~~~~~~~
/home/antoine/arrow/python/pyarrow/lib.cpp:148536:26: note: in expansion of macro ‘__PYX_VERIFY_RETURN_INT’
                 case  1: __PYX_VERIFY_RETURN_INT(enum  arrow::Type::type, digit, digits[0])
                          ^

It seems the -1 return value from __Pyx_PyInt_As_enum____arrow_3a__3a_Type_3a__3a_type is supposed to denote an error... But -1 is never checked for by callers, instead they call PyErr_Occurred. So perhaps returning 0 would work just as well.

@pitrou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

It seems that by defining the following macro:

#if defined(__cplusplus) && __cplusplus >= 201103
// Avoid conversion warnings with gcc and -Wconversion
#include <type_traits>
#define __PYX_SAFE_INT_CAST(target_type, value) \
  static_cast<target_type>(static_cast<typename std::make_signed<target_type>::type>(value))
#else
#define __PYX_SAFE_INT_CAST(target_type, value) \
  (target_type) value
#endif

Replacing the given casts by __PYX_SAFE_INT_CAST(target_type, -1) then makes the warning disappears. Though how robust this solution is I cannot say.

@pitrou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Another possibility is this (force the warning off temporarily):

#if defined(__GNUC__) && defined(__cplusplus) && __cplusplus >= 201103

// Avoid conversion warnings with gcc and -Wconversion
template <typename DestType, typename SrcType>
DestType __pyx_safe_int_cast(SrcType value) {
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wconversion"
  return static_cast<DestType>(value);
#pragma GCC diagnostic pop
}
#define __PYX_SAFE_INT_CAST(target_type, value) \
  __pyx_safe_int_cast<target_type>(value)

#else

#define __PYX_SAFE_INT_CAST(target_type, value) \
  (target_type) value

#endif
@scoder

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

I definitely prefer a proper C++ solution over mingling with the warnings of a specific compiler (and not others), although doing the latter would still be a reasonable fallback for older C++ versions. We are also clearly missing a test that uses C++ enums in various integer contexts. There are a couple of C-enum tests (e.g. this or other tests/*/*enum*) that give similar warnings with -Wconversion.

Would you mind writing up a PR? Maybe you can extend the cpp_enums test.

I also think we should try enabling -Wconversion for at least some of Cython's own test suite (the overflow checks might be challenging), and see what that gives. Might uncover some more potential issues, or at least code that gets in the way of users who want warning-free code with that option. Cython should always strive to generate warnings-free code, and I can understand that some users want to enable specifically -Wconversion for their code.

This message (only in C++ mode) is also truly worrying:

enumboolctx.cpp:2750:34: warning: the result of the conversion is unspecified because ‘-1’ is outside the range of type ‘Truth’ [-Wconversion]
             return (enum Truth) -1;

According to an SO answer, this is even undefined behaviour starting from C++17, meaning, it might lead to silent misbehaviour there, eventually.

@pitrou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

I definitely prefer a proper C++ solution over mingling with the warnings of a specific compiler

I wouldn't call what I posted above a "proper solution". It silences the warning, but I don't think it suppresses the undefined behaviour.

Is there a reason -1 has to be used for error return here?

@scoder

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

Is there a reason -1 has to be used for error return here?

No, it should just be something unique and not so likely to occur in practice (which would actually rather suggest something like INT_MIN than -1). That's impossible to decide out of thin air for an arbitrary user defined enum. This is strictly internal code without compatibility constraints, though, so any suggestion for improvement would be appreciated.

One idea is to allow users to define their own (efficient) exception value, e.g.

cdef enum xyz except? RARE_VALUE:
    DEFAULT, RARE_VALUE, OTHER

Maybe with a default that just uses the first one with a PyErr_Occurred() check (i.e. except? FIRST). For module internal enums, users could then also add a dedicated exception return value that is not used for anything else, and declare it as except EXC_VAL, with out the ?.

@robertwb

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

-1 is a conventional error value for CPython, which is probably why it was used here, but perhaps we could just return any value when PyErr_Occurred is called. (This assumes that we have a set of values to choose from, but that's typically the case.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.