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

Only use [[fallthrough]] in C++11 and beyond #1930

Closed
wants to merge 1 commit into from

Conversation

jdemeyer
Copy link
Contributor

In this code:

  #ifdef __cplusplus
    #if __has_cpp_attribute(fallthrough)
      #define CYTHON_FALLTHROUGH [[fallthrough]]
    #elif __has_cpp_attribute(clang::fallthrough)
      #define CYTHON_FALLTHROUGH [[clang::fallthrough]]
    #endif
  #endif

Cython checks for the presence of the fallthrough attribute but that doesn't mean that the C++11 attribute syntax [[fallthrough]] is understood.

GCC 7.2.0 for example gives warnings when compiling with -std=gnu++98:

build/cythonized/sage/libs/lcalc/lcalc_Lfunction.cpp:515:48: warning: c++11 attributes only available with -std=c++11 or -std=gnu++11
       #define CYTHON_FALLTHROUGH [[fallthrough]]
                                                ^
build/cythonized/sage/libs/lcalc/lcalc_Lfunction.cpp:3428:9: note: in expansion of macro 'CYTHON_FALLTHROUGH'
         CYTHON_FALLTHROUGH;
         ^~~~~~~~~~~~~~~~~~

@dalcinl
Copy link
Member

dalcinl commented Oct 13, 2017

@jdemeyer Shouldn't you keep the outer ifdef __cplusplus?

#ifdef __cplusplus
  #if __cplusplus >= 201103L
    ....
  #endif
#endif

@dalcinl
Copy link
Member

dalcinl commented Oct 13, 2017

Also, it would be good to have this in an upcoming patch release of 0.27.2, thus the merge branch should be retargeted?.

@scoder
Copy link
Contributor

scoder commented Oct 13, 2017

the merge branch should be retargeted?

Not sure how Robert thinks about this, but I generally prefer pull requests for master. Backporting or cherry picking to maintenance branches should generally be decided explicitly by those who maintain the branch.

@dalcinl
Copy link
Member

dalcinl commented Oct 13, 2017

I patched the petsc4py tarball, a new build is triggered: conda-forge/petsc4py-feedstock#4 Let see if this patch fixes the issue with clang on Travis-CI.

@dalcinl
Copy link
Member

dalcinl commented Oct 13, 2017

@scoder Sorry, I'm OK with that. It is just that I'm used to a different workflow.

@scoder scoder added this to the 0.27.2 milestone Oct 13, 2017
@scoder scoder closed this in 207d694 Oct 13, 2017
@robertwb
Copy link
Contributor

My workflow is generally to patch the release then merge back into master over cherry-picking (for stuff that I know is safe) but given that we historically haven't maintained release branches vs. just issued new releases (deciding on a major vs. minor bump based on its contents) and have only one "active" branch except when actively preparing a release the difference is small.

scoder added a commit that referenced this pull request Oct 14, 2017
…arnings, assuming that "__has_attribute()" is widely supported these days. At least GCC 5.x and clang seem to have it.

Closes #1930.
scoder added a commit that referenced this pull request Oct 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants