Fixed unknown pragma warnings for _BitScanReverse. #259
Conversation
Changed to only define the MSVC implementations for clz and clzll if the builtins are not available to avoid warnings about an unknown #pragma for "intrinsic".
| // otherwise support __builtin_clz and __builtin_clzll, so | ||
| // only define FMT_BUILTIN_CLZ using the MSVC intrinsics | ||
| // if the clz and clzll builtins are not available. | ||
| # if !defined(FMT_BUILTIN_CLZ) |
There was a problem hiding this comment.
Why not merge this #ifdef with _MSC_VER check above for simplicity and to avoid intrin.h inclusion?
There was a problem hiding this comment.
That's what I originally had, but I was being overly paranoid and worried about "what if there's a compiler with __builtin_clz support, but not __builtin_clzll, or the other way around?" and #if defined(_MSC_VER) && (!defined(FMT_BUILTIN_CLZ) || !defined(FMT_BUILTIN_CLZLL)) was difficult for me to read and validate that it was correct.
There was a problem hiding this comment.
I suppose I could also write a "test" in util-test.cc that emits a #error if FMT_BUILTIN_CLZ and FMT_BUILTIN_CLZLL macros are not defined in the cases where it's obvious they should be supported, but avoiding warnings makes it cumbersome to see that in the real code.
There was a problem hiding this comment.
I think it's safe to just check for FMT_BUILTIN_CLZLL. FMT_BUILTIN_CLZ is just an optimization and although I wouldn't expect compilers to provide one and not the other, it shouldn't be critical.
|
Thanks for another useful PR. Mostly looks good, just one minor comment above. |
…able. This is mainly just to avoid including intrin.h unnecessarily.
Fixed unknown pragma warnings for _BitScanReverse.
|
Thanks! |
Changed to only define the MSVC implementations for clz and clzll if the builtins are not available to avoid warnings about an unknown #pragma for "intrinsic".