On Wed, Mar 12, 2025 at 9:23 AM Jonathan Wakely <jwak...@redhat.com> wrote:
> 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.
>
Any particular reason to use this as a solution, as compared to
alternatives with implementation defined function?
I find the latter much more understandable, and while this allows us to
avoid checking if user specified types in exactly same order,
passing <basic_string_view, char> to check_dynamic_spec will still perform
checking.
>
> >>
> >> + // 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
> >>
>
>
