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

Cython 0.29 broke support for C++ enum classes #2749

Open
orivej opened this Issue Dec 7, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@orivej
Copy link
Contributor

orivej commented Dec 7, 2018

Before be4ca1d (a fix for #2186), on this input:

cpdef enum E "E":
    A

Cython was generating this:

/* CIntToPy */
static CYTHON_INLINE PyObject* __Pyx_PyInt_From_enum__E(enum E value) {
    const enum E neg_one = (enum E) -1, const_zero = (enum E) 0;

Since 6079a5b it is generating this:

/* CIntToPy */
static CYTHON_INLINE PyObject* __Pyx_PyInt_From_enum__E(enum E value) {
    const enum E neg_one = (enum E) ((enum E) 0 - (enum E) 1), const_zero = (enum E) 0;

This works for C enums, but fails to compile for C++ enum classes (https://godbolt.org/z/NY8PJT):

enum class E { X };
const enum E neg_one_class_28 = (enum E) -1;
const enum E neg_one_class_29 = (enum E) ((enum E) 0 - (enum E) 1);
<source>:3:54: error: invalid operands to binary expression ('enum E' and 'enum E')
const enum E neg_one_class_29 = (enum E) ((enum E) 0 - (enum E) 1);
                                          ~~~~~~~~~~ ^ ~~~~~~~~~~
@orivej

This comment has been minimized.

Copy link
Contributor

orivej commented Dec 7, 2018

Here is a complete example.
e.hpp:

enum class E { A, B };

e.pyx:

cdef extern from "e.hpp" namespace "E":
    cpdef enum E "E":
        A
        B
@scoder

This comment has been minimized.

Copy link
Contributor

scoder commented Dec 12, 2018

Ok, this is really getting cumbersome … :)
What other options do we have that C and/or C++ compilers would accept without complaining all too loudly? Maybe (enum E) (0 - 1), without the second cast ?

@scoder scoder added this to the 0.29.2 milestone Dec 12, 2018

@scoder scoder added C++ defect labels Dec 12, 2018

@orivej

This comment has been minimized.

Copy link
Contributor

orivej commented Dec 12, 2018

I think that (enum E) (0 - 1) would be the same as (enum E) -1, as was before #2186. I can not reproduce the warning mentioned in that issue, even though I have tried various versions of gcc and clang with -Wall, with enum and enum class: https://godbolt.org/z/buBUI9

@orivej

This comment has been minimized.

Copy link
Contributor

orivej commented Dec 12, 2018

Apparently it is a GCC bug that (enum E) -1 generates a warning with -Wconversion, since it is documented that it should "not warn for explicit casts like abs((int) x) and ui = (unsigned) -1". Moreover, the current workaround does not help gcc 5.5 and earlier, as demonstrated in https://godbolt.org/z/DlXIfA.

I propose to explicitly silence this warning with:

#ifdef __GNUC__
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wconversion"
#endif

const enum E a = (enum E) -1;

#ifdef __GNUC__
#pragma GCC diagnostic pop
#endif

@scoder scoder modified the milestones: 0.29.2, 0.29.3 Dec 14, 2018

orivej added a commit to orivej/cython that referenced this issue Dec 24, 2018

Fix support for C++ enum classes
The current code fails to compile with:

<source>:3:54: error: invalid operands to binary expression ('enum E' and 'enum E')
const enum E neg_one_class_29 = (enum E) ((enum E) 0 - (enum E) 1);
                                          ~~~~~~~~~~ ^ ~~~~~~~~~~

This change reverts to the code that was before cython#2186 but silences erroneous GCC warning enabled by -Wconversion (which is not a part of -Wall).

Fixes cython#2749

@orivej orivej referenced a pull request that will close this issue Dec 24, 2018

Open

Fix support for C++ enum classes #2767

@kblomqvist

This comment has been minimized.

Copy link

kblomqvist commented Dec 27, 2018

I thought that enum class is not supported yet. See, #1603.

@orivej

This comment has been minimized.

Copy link
Contributor

orivej commented Dec 27, 2018

They are not fully supported: for example, it seems difficult to use enum class variables as function arguments; but what Cython already has is enough to wrap them. The original example in #1603 can be encoded like this:

cdef extern from "headername.h" namespace "lol::rofl":
    cdef enum rofl:
        SOMETHING
        MOAR

cdef void f(int arg):
    print(arg)

f(<int>rofl.MOAR)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment