-
Notifications
You must be signed in to change notification settings - Fork 86
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
Extend fallthrough safety to LLVM in gmpy2_mpmath.c #502
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be gcc/clang-specific.
Not a surprise, this will break CI on Windows:
D:\a\gmpy\gmpy\src\gmpy2_mpmath.c(290): warning C4013: '__attribute__' undefined; assuming extern returning int
D:\a\gmpy\gmpy\src\gmpy2_mpmath.c(290): error C2065: 'fallthrough': undeclared identifier
D:\a\gmpy\gmpy\src\gmpy2_mpmath.c(297): error C2065: 'fallthrough': undeclared identifier
D:\a\gmpy\gmpy\src\gmpy2_mpmath.c(300): error C2065: 'fallthrough': undeclared identifier
src/gmpy2_mpmath.c
Outdated
@@ -287,17 +287,17 @@ Pympz_mpmath_create_fast(PyObject *self, PyObject *const *args, Py_ssize_t nargs | |||
switch (nargs) { | |||
case 4: | |||
rnd = PyString_1Char(args[3]); | |||
/* fallthrough */ | |||
__attribute__((fallthrough)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attribute
Could you point on page in C99 or C11 where this keyword was defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GCC talks about the fallthrough attribute here. clang also supports it, but see my message below.
The current fallthrough comments only suppress warnings for GCC. `__attribute__((fallthrough));` works for both GCC (>=7) and LLVM. C23 will introduce `[[fallthrough]]`.
@skirpichev - Most projects I've worked with have opted to use a preprocessor macro to handle compatibility issues. I've added one here, but note that you'll only get the benefits of the protections by using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I still don't see value in this pr. Commentary is recognized by gcc and clang. It's harmless and useless for M$ VC (BTW, does it has -Wimplicit-fallthrough
analog?).
@skirpichev - clang does not recognize comments as having semantics, so the code as-is will fail to compile using clang with |
Ah, then it makes some sense.
(only with -Werror) |
The current fallthrough comments only suppress warnings for GCC.
__attribute__((fallthrough));
works for both GCC (>=7) and LLVM. C23 will introduce[[fallthrough]]
.