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

Add support for _BitInt on clang #4072

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Arghnews
Copy link
Contributor

Issue #4007

Only enabled for clang >= 14 as that's all that supports _BitInt at the moment

We need a forwarding reference for map to be a "more specialized" match; const T&, T etc. are ambiguous.

I'm not too familiar, would there be a better way to do this by adding an entry into enum class type for _BitInts? Or this is fine?


Fyi this worked in version 9 because of this catch-all overload:

fmt/include/fmt/core.h

Lines 1506 to 1516 in a337011

template <typename T, typename U = remove_cvref_t<T>,
FMT_ENABLE_IF(!is_string<U>::value && !is_char<U>::value &&
!std::is_array<U>::value &&
!std::is_pointer<U>::value &&
!has_format_as<U>::value &&
(has_formatter<U, Context>::value ||
has_fallback_formatter<U, char_type>::value))>
FMT_CONSTEXPR FMT_INLINE auto map(T&& val)
-> decltype(this->do_map(std::forward<T>(val))) {
return do_map(std::forward<T>(val));
}

Which was then subsequently narrowed down to extended FP types (which "broke" the accidental support for this):

fmt/include/fmt/base.h

Lines 1527 to 1537 in 6a192f8

// is_fundamental is used to allow formatters for extended FP types.
template <typename T, typename U = remove_const_t<T>,
FMT_ENABLE_IF(
(std::is_class<U>::value || std::is_enum<U>::value ||
std::is_union<U>::value || std::is_fundamental<U>::value) &&
!has_to_string_view<U>::value && !is_char<U>::value &&
!is_named_arg<U>::value && !std::is_integral<U>::value &&
!std::is_arithmetic<format_as_t<U>>::value)>
FMT_MAP_API auto map(T& val) -> decltype(FMT_DECLTYPE_THIS do_map(val)) {
return do_map(val);
}

@Arghnews Arghnews force-pushed the support_bitint branch 3 times, most recently from 903edfd to 3666cee Compare July 13, 2024 18:35
include/fmt/base.h Outdated Show resolved Hide resolved
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!

@@ -427,6 +427,14 @@ enum class uint128_opt {};
template <typename T> auto convert_for_visit(T) -> monostate { return {}; }
#endif

#ifndef FMT_USE_BITINT
# if FMT_CLANG_VERSION && FMT_CLANG_VERSION >= 1400
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified to

#  if FMT_CLANG_VERSION >= 1400

Comment on lines +1495 to +1500
#if FMT_USE_BITINT
# pragma clang diagnostic push
# pragma clang diagnostic ignored "-Wbit-int-extension"
template <class T, int N = 0> struct is_bitint : std::false_type {};
template <int N> struct is_bitint<_BitInt(N)> : std::true_type {};
template <int N> struct is_bitint<unsigned _BitInt(N)> : std::true_type {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this out of the mapper, next to the _BitInt detection above.

#if FMT_USE_BITINT
# pragma clang diagnostic push
# pragma clang diagnostic ignored "-Wbit-int-extension"
template <class T, int N = 0> struct is_bitint : std::false_type {};
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be defined outside of conditional block so that the check could be used on all platforms.

Comment on lines +2452 to +2454
template <> struct formatter<BitIntS7> : formatter<int8_t> {};

template <> struct formatter<BitIntU32> : formatter<uint32_t> {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Defining formatters for _BitInt should be disallowed.

template <int N> struct is_bitint<unsigned _BitInt(N)> : std::true_type {};

template <class T, FMT_ENABLE_IF(is_bitint<remove_cvref_t<T>>::value)>
FMT_MAP_API auto map(T&& val) -> decltype(val) {
Copy link
Contributor

Choose a reason for hiding this comment

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

_BitInt should be converted to appropriate integer type.

Copy link
Contributor Author

@Arghnews Arghnews Jul 19, 2024

Choose a reason for hiding this comment

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

Are we assuming the maximum width for a _BitInt is 128 bits here, so we can widen to any of the native integer types (including __int128)?

I think officially this is what clang supports according to BITINT_MAXWIDTH for c++ (although from the link in that 5th bullet point, it seems this is more due to a limitation than a design choice. Doesn't seem to be defined for 18 wherever I try it either)

Although for C(23), it supports up to 8388608
https://godbolt.org/z/jzfGYoY6a

@vitaut

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can assume that for now and in any case for smaller sizes we should convert to integers for efficiency instead of going through the formatter extension.

@vitaut
Copy link
Contributor

vitaut commented Jul 27, 2024

@Arghnews, do you plan to update this PR or shall we close it for now (can be reopened later)?

@Arghnews
Copy link
Contributor Author

@Arghnews, do you plan to update this PR or shall we close it for now (can be reopened later)?

@vitaut Hi, sorry been busy and now I'm on holiday for 2 weeks, but will update once back and implement your feedback. I'd say leave it open and I'll update in 2-3 weeks, but it's up to you (can reopen of course). Cheers!

@vitaut
Copy link
Contributor

vitaut commented Jul 27, 2024

No hurry. Let's keep it open and enjoy your holiday!

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

3 participants