-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Cleanup warnings with nvhpc/21.9. #2582
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.
Thanks for the PR.
include/fmt/format.h
Outdated
@@ -967,14 +967,17 @@ FMT_CONSTEXPR20 inline auto count_digits(uint64_t n) -> int { | |||
template <int BITS, typename UInt> | |||
FMT_CONSTEXPR auto count_digits(UInt n) -> int { | |||
#ifdef FMT_BUILTIN_CLZ | |||
if (num_bits<UInt>() == 32) | |||
if (num_bits<UInt>() == 32) { |
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.
Let's wrap the condition in const_check
instead.
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.
template <int BITS, typename UInt>
FMT_CONSTEXPR auto count_digits(UInt n) -> int {
#ifdef FMT_BUILTIN_CLZ
if (const_check(num_bits<UInt>() == 32))
return (FMT_BUILTIN_CLZ(static_cast<uint32_t>(n) | 1) ^ 31) / BITS + 1;
#endif
int num_digits = 0;
doesn't squash the warning.
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.
That's unfortunate but I think the suppression with extra blocks is too messy. Is there another way to do this?
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.
eafdd3e is another way -- I wouldn't say it's a particularly nice way...
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.
Yeah, this is clearly worse =(. Does moving the definition of num_digits
to the beginning of the function suppress the warning?
int num_digits = 0;
#ifdef FMT_BUILTIN_CLZ
if (const_check(num_bits<UInt>() == 32))
return (FMT_BUILTIN_CLZ(static_cast<uint32_t>(n) | 1) ^ 31) / BITS + 1;
#endif
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.
Nice idea, but that just gives
warning #128-D: loop is not reachable
on the do {
line. And I would be a little worried about other compilers giving an unused variable warning too...
I pushed yet another attempt, with an immediately executed lambda, which seems to work...
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 looks better but please fix the CI issues.
Thanks |
* Cleanup warnings with nvhpc/21.9. * Move __NVCOMPILER check. * Be more explicit. * Immediately executed lambda. * Fix shadowing warning.
This squashes a couple of warnings when building fmt 8.0.1 with version 21.9 of the NVIDIA HPC compiler.