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

[Jonathan Wakely]

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.

This passes std/format/* tests, I'm running the full testsuite now.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.txt
Type: text/x-patch
Size: 8376 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20250312/4a1b8570/attachment.bin>