Refactor error disabling #80
Conversation
setup.py
Outdated
| "backports.zoneinfo._czoneinfo", sources=["lib/zoneinfo_module.c"], | ||
| "backports.zoneinfo._czoneinfo", | ||
| sources=["lib/zoneinfo_module.c"], | ||
| extra_compile_args=["-std=c99"], |
There was a problem hiding this comment.
I'm not quite sure how to do this in a cross-platform way, because this works for gcc and clang but not for MSVC, which warns about an unknown argument (though the command still succeeds).
There was a problem hiding this comment.
Here's some prior art: python/cpython@0b7829e#diff-2e01a9f45ceaa02248341ccd350bc7c0
MSVC is primarily meant to be a C++ compiler so it only has c++ mode switches, the standard of C it implements is basically C99 I think. So just leaving it without a flag should be fine.
There was a problem hiding this comment.
Does that work if someone is using cygwin or mingw or something?
There was a problem hiding this comment.
Good question, apparently mingw overrides sys.platform to be mingw when using gcc so it should work there:
https://github.com/msys2/MINGW-packages/blob/abd06ca92d876b9db05dd65f27d71c4ebe2673a9/mingw-w64-python2/0410-MINGW-build-extensions-with-GCC.patch#L54
Not sure about cygwin though I'd imagine it also overrides sys.platform for this reason.
There was a problem hiding this comment.
Hm, looks like mingw and cygwin return mingw and cygwin, respectively. I'm not familiar enough with Windows to know if there is a gcc for that platform, but I guess it's pretty safe to do it this way now, I suppose.
I guess the real question is how important is this. It seems like the worst thing that happens in the common case is that it raises a warning on Windows, but if someone does have a Windows system out there running gcc, I think this turns it into a hard failure.
I guess I could do the sys.platform check and then as a fallback put the disabling pragma under a check against __STDC_VERSION__ >= 199901L (since that's the only part that requires C99.
There was a problem hiding this comment.
While there's no way to run gcc without mingw or cygwin, clang can actually be built natively and made available available on Windows. Though even there they provide a utility to make the toolchain compatible with MSVC: https://clang.llvm.org/docs/MSVCCompatibility.html#msvc-compatibility
I think this might not be worth handling with the preprocessor directive, may as well wait to see if anyone brings it up as a real problem.
There was a problem hiding this comment.
The -std=c99 is really only needed on gcc because the only C99 feature we're using is _Pragma, which is only in the GCC branch of the conditional compile, so I suppose we're safe.
There was a problem hiding this comment.
clang sets __GNUC__ since it implements gcc compiler extensions.
|
@ammaraskar What do you think of this? |
|
Aah I see what you mean about the verbosity, this seems good. It might make a good addition to |
GCC-specific pragmas will raise unknown pragma warnings on MSVC; rather than adding #ifdefs around the individual pragmas, this defines a macro that uses the pragmas on GCC but is a no-op on other compilers. This requires using at least c99, which is when _Pragma was introduced.
This is no longer necessary, since we are now conditionally using pragmas to disable warnings on a per-compiler basis.
97370e1 to
7129b1a
Compare
This uses a macro to use the "disable this warning on GCC" pragmas only on GCC, to avoid "unknown pragma" warnings on other compilers.