[PATCH v2] libstdc++: Optimize basic_format_parse_context::check_dynamic_spec

[Tomasz Kaminski]

On Wed, Mar 12, 2025 at 11:06 AM Jonathan Wakely <jwakely@redhat.com> wrote:

> On 12/03/25 09:33 +0100, Tomasz Kaminski wrote:
> >On Wed, Mar 12, 2025 at 9:23 AM Jonathan Wakely <jwakely@redhat.com>
> wrote:
> >
> >> On Wed, 12 Mar 2025 at 07:59, Tomasz Kaminski <tkaminsk@redhat.com>
> wrote:
> >> >
> >> >
> >> >
> >> > On Tue, Mar 11, 2025 at 11:46 PM Jonathan Wakely <jwakely@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.
>
> No particular reason, no.
>
> Here's a patch implementing the alternative approach, which does seem
> cleaner.
>
LGTM.
Nice touch on making __check_dynamic_spec consteval, so it never get symbol
emitted.

>
> This passes std/format/* tests, I'm running the full testsuite now.
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20250312/9372c8d3/attachment-0001.htm>