bpo-41617: Fix pycore_bitutils.h to support clang 3.0#21949
Conversation
|
Please fix compile errors :) |
|
Oh, __has_builtin() was added to GCC 10 and so it works on Fedora 32, but not on Ubuntu 18.04 which uses GCC 7.5.0. |
|
Yeah, that's why the approach of defining a null #ifdef __has_builtin
#if __has_builtin(__builtin_bswap16)
// do thing using __builtin_bswap16
#endif
#endif |
|
Oh oh, the doc job failed because of setuptools 50.0 ... |
|
@jmroot: Would you mind to review my updated PR? I abandoned my first idea and my PR now defines _Py_has_builtin() which is defined as |
|
My PR is very similar to PR #21942, but I want to define the macro in pymacro.h, so |
__builtin_bswap16() is not available in LLVM clang 3.0. Add a private _Py_has_builtin() macro: use __has_builtin() if available, otherwise always return 0.
|
PR rebased to retrieve https://bugs.python.org/issue41685 CI fix. |
| _Py_bswap32(uint32_t word) | ||
| { | ||
| #ifdef _PY_HAVE_BUILTIN_BSWAP | ||
| #if _Py_has_builtin(__builtin_bswap32) || defined(_PY_HAVE_BUILTIN_BSWAP) |
There was a problem hiding this comment.
I have the feeling that _Py_has_builtin(__builtin_bswap32) should be used to correctly position _PY_HAVE_BUILTIN_BSWAP instead of being used here. That way if _PY_HAVE_BUILTIN_BSWAP is mentioned elsewhere, it will have the correct value.
There was a problem hiding this comment.
_PY_HAVE_BUILTIN_BSWAP is used to get the optimization on GCC older than GCC 10. __has_feature() was added very recently in GCC: in GCC 10.
|
_Py_has_builtin() may be more future-proof and more flexible to test the availability of compiler features. But I also proposed a dummy fix without _Py_has_builtin(): PR #22042. I have a preference for _Py_has_builtin(). |
|
I merged the simple PR #22042 instead. |
Apologies, I never received an email notification for this comment for some reason, so I only saw it just now. |
__builtin_bswap16() is not available in LLVM clang 3.0.
https://bugs.python.org/issue41617