On Tue, Mar 11, 2025 at 11:46 PM Jonathan Wakely <jwak...@redhat.com> wrote:
> This change makes the consteval function slightly faster to compile.
> Instead of keeping the counts in an array and looping over that array,
> we can just keep a sum of how many valid types are present, and check
> that it equals the total number of types in the pack. We can also avoid
> doing the check entirely for the common cases where the call comes from
> check_dynamic_spec_integer or check_dynamic_spec_string, because those
> always use valid lists of types.
>
> The diagnostic is slightly worse now, because there's only a single
> "invalid template argument types" string that appears in the output,
> where previously we had either "non-unique template argument type" or
> "disallowed template argument type" depending on the failure mode.
>
> Given that most users will never use this function directly, and
> probably won't use invalid types anyway, the inferior diagnostic seems
> acceptable.
>
> libstdc++-v3/ChangeLog:
>
> * include/std/format (basic_format_parse_context::__check_types):
> New variable template and partial specializations.
> (basic_format_parse_context::__check_dynamic_spec_types):
> Simplify for faster compilation.
> (basic_format_parse_context::check_dynamic_spec): Only use
> __check_dynamic_spec_types when __check_types is true. Use it as
> the condition for a constexpr if statement instead of as the
> initializer for a constexpr variable.
> (basic_format_parse_context::check_dynamic_spec_string): Use
> _CharT instead of char_type consistently.
> ---
>
> Tested x86_64-linux.
>
> libstdc++-v3/include/std/format | 81 +++++++++++++++++++--------------
> 1 file changed, 47 insertions(+), 34 deletions(-)
>
> diff --git a/libstdc++-v3/include/std/format
> b/libstdc++-v3/include/std/format
> index 0d6cc7f6bef..6269cbb80e6 100644
> --- a/libstdc++-v3/include/std/format
> +++ b/libstdc++-v3/include/std/format
> @@ -305,41 +305,51 @@ namespace __format
> constexpr void
> check_dynamic_spec_string(size_t __id) noexcept
> {
> - check_dynamic_spec<const char_type*,
> basic_string_view<_CharT>>(__id);
> + check_dynamic_spec<const _CharT*, basic_string_view<_CharT>>(__id);
> }
>
> private:
> - // Check the Mandates: condition for check_dynamic_spec<Ts...>(n)
> + template<typename _Tp, typename... _Ts>
> + static constexpr bool __once = (is_same_v<_Tp, _Ts> + ...) == 1;
> +
> + // True if we need to call __check_dynamic_spec_types for the pack
> Ts
> + template<typename... _Ts>
> + static constexpr bool __check_types = sizeof...(_Ts) > 0;
> + // The pack used by check_dynamic_spec_integral is valid, don't
> check it.
> + // FIXME: simplify these when PR c++/85282 is supported.
> + template<typename _Tp>
> + static constexpr bool
> + __check_types<_Tp, unsigned, long long, unsigned long long>
> + = ! is_same_v<_Tp, int>;
>
These seem to produce wrong answer (false) if the user called
check_dynamic_spec<basic_string_view<_CharT>, unsigned, long long, unsigned
long long>,
which is a valid set of types.
> + // The pack used by check_dynamic_spec_string is valid, don't check
> it.
> + template<typename _Tp>
> + static constexpr bool
> + __check_types<_Tp, basic_string_view<_CharT>>
> + = ! is_same_v<const _CharT*, _Tp>;
>
As above check_dynamic_spec<int, basic_string_view<_CharT>>.
I think much batter solution would be to have __check_dynamic_spec<Ts...>
private function,
that would not perform the check at all and will be called without checking
from check_dynamic_spec_integrral, e.t.c.
And in check_dynamic_spec, we would do __check_dynamic_spec_types<_Ts...>()
and sizeof..(Ts) > 0, before
calling __check_dynamic_spec.
+
> + // Check the Mandates: conditions for check_dynamic_spec<Ts...>(n)
> template<typename... _Ts>
> static consteval bool
> __check_dynamic_spec_types()
> {
> - if constexpr (sizeof...(_Ts))
> - {
> - int __counts[] = {
> - (is_same_v<bool, _Ts> + ...),
> - (is_same_v<_CharT, _Ts> + ...),
> - (is_same_v<int, _Ts> + ...),
> - (is_same_v<unsigned, _Ts> + ...),
> - (is_same_v<long long, _Ts> + ...),
> - (is_same_v<unsigned long long, _Ts> + ...),
> - (is_same_v<float, _Ts> + ...),
> - (is_same_v<double, _Ts> + ...),
> - (is_same_v<long double, _Ts> + ...),
> - (is_same_v<const _CharT*, _Ts> + ...),
> - (is_same_v<basic_string_view<_CharT>, _Ts> + ...),
> - (is_same_v<const void*, _Ts> + ...)
> - };
> - int __sum = 0;
> - for (int __c : __counts)
> - {
> - __sum += __c;
> - if (__c > 1)
> - __invalid_dynamic_spec("non-unique template argument
> type");
> - }
> - if (__sum != sizeof...(_Ts))
> - __invalid_dynamic_spec("disallowed template argument
> type");
> - }
> + // The types in Ts... are unique. Each type in Ts... is one of
> + // bool, char_type, int, unsigned int, long long int,
> + // unsigned long long int, float, double, long double,
> + // const char_type*, basic_string_view<char_type>, or const
> void*.
> + unsigned __sum = __once<bool, _Ts...>
> + + __once<char_type, _Ts...>
> + + __once<int, _Ts...>
> + + __once<unsigned int, _Ts...>
> + + __once<long long int, _Ts...>
> + + __once<unsigned long long int, _Ts...>
> + + __once<float, _Ts...>
> + + __once<double, _Ts...>
> + + __once<long double, _Ts...>
> + + __once<const char_type*, _Ts...>
> + + __once<basic_string_view<char_type>, _Ts...>
> + + __once<const void*, _Ts...>;
> + if (__sum != sizeof...(_Ts))
> + __invalid_dynamic_spec("invalid template argument types");
> return true;
> }
>
> @@ -4341,12 +4351,15 @@ namespace __format
> // _GLIBCXX_RESOLVE_LIB_DEFECTS
> // 4142. check_dynamic_spec should require at least one type
> static_assert(sizeof...(_Ts) >= 1);
> - // This call enforces the Mandates: condition that _Ts contains
> valid
> - // types and each type appears at most once. It could be a
> static_assert
> - // but this way failures give better diagnostics, due to calling the
> - // non-constexpr __invalid_dynamic_spec function.
> - [[maybe_unused]]
> - constexpr bool __ok = __check_dynamic_spec_types<_Ts...>();
> +
> + if constexpr (__check_types<_Ts...>)
> + if constexpr (__check_dynamic_spec_types<_Ts...>())
> + {
> + // The call above enforces the Mandates: condition that _Ts
> + // contains valid types and each type appears at most once.
> + // Checking it in `if constexpr` means that failures give
> better
> + // diagnostics than if we used it as a static_assert condition.
> + }
>
> if consteval {
> if (__id >= _M_num_args)
> --
> 2.48.1
>
>
