On Wed, 12 Mar 2025 at 07:59, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> 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 variable template __check_types means "do we need to check the
types", so false is the correct answer here. We do not need to check
the types for this list of types, because we know it's a valid set of
types.
The variable template is used to decide whether to call
__check_dynamic_spec_types or to skip calling it.
>>
>> + // 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.
Yes I considered that, and that would also work.
>> +
>> + // 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
>>
