On Wed, 12 Mar 2025 at 10:47, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> On Wed, Mar 12, 2025 at 11:06 AM Jonathan Wakely <jwak...@redhat.com> wrote:
>>
>> On 12/03/25 09:33 +0100, Tomasz Kaminski wrote:
>> >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.
>>
>> 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.
I suppose we could even add [[__gnu__::__always_inline__]] to all
three of check_dynamic_spec, check_dynamic_spec_integral and
check_dynamic_spec_string.
That would slightly benefit -O0 builds but I don't think it matters.
std::format is already going to be very slow without optimization.
>>
>>
>> This passes std/format/* tests, I'm running the full testsuite now.
>>
>>
