Skip to content
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

exclude fallback functions when FMT_BUILTIN_CLZ(LL) is not defined #2434

Merged
merged 4 commits into from
Jul 25, 2021

Conversation

bodomartin
Copy link
Contributor

Trivial change to format.h to exclude fallback functions when FMT_BUILTIN_CLZ and FMT_BUILTIN_CLZLL
are not available. Happens to me on windows clangcl msvc codegen

Signed-off-by: Bodo Martin <bodo.martin@aci.uni-heidelberg.de>
Copy link
Contributor

@vitaut vitaut left a 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. LGTM but please address inline comments.

@@ -918,6 +918,7 @@ FMT_CONSTEXPR inline auto count_digits(uint128_t n) -> int {

// It is a separate function rather than a part of count_digits to workaround
// the lack of static constexpr in constexpr functions.
#ifdef FMT_BUILTIN_CLZLL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the #ifdef right before the comment:

#ifdef FMT_BUILTIN_CLZLL
// It is a separate function rather than a part of count_digits to workaround
// the lack of static constexpr in constexpr functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -964,6 +966,7 @@ template <> auto count_digits<4>(detail::fallback_uintptr n) -> int;

// It is a separate function rather than a part of count_digits to workaround
// the lack of static constexpr in constexpr functions.
#ifdef FMT_BUILTIN_CLZ
Copy link
Contributor

@vitaut vitaut Jul 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@vitaut vitaut marked this pull request as ready for review July 25, 2021 17:36
@vitaut vitaut merged commit 0544a22 into fmtlib:master Jul 25, 2021
@vitaut
Copy link
Contributor

vitaut commented Jul 25, 2021

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants