From 2e64d7f796e4dfc2e4c15bef994f038ff275c34d Mon Sep 17 00:00:00 2001 From: "A. Jiang" Date: Tue, 30 Apr 2024 01:20:04 +0800 Subject: [PATCH 1/5] Fix handling of replacement field when format-spec is absent --- stl/inc/format | 16 +++-- .../test.cpp | 65 ++++++++++++++++++- 2 files changed, 74 insertions(+), 7 deletions(-) diff --git a/stl/inc/format b/stl/inc/format index 00c0aa1e2f4..7e2e9623839 100644 --- a/stl/inc/format +++ b/stl/inc/format @@ -3537,6 +3537,7 @@ struct _Default_arg_formatter { _OutputIt _Out; basic_format_args<_Context> _Args; _Lazy_locale _Loc; + basic_format_parse_context<_CharT>& _Parse_ctx; template _OutputIt operator()(_Ty _Val) && { @@ -3544,7 +3545,6 @@ struct _Default_arg_formatter { } _OutputIt operator()(basic_format_arg<_Context>::handle _Handle) && { - basic_format_parse_context<_CharT> _Parse_ctx({}); auto _Format_ctx = _Context::_Make_from(_STD move(_Out), _Args, _Loc); _Handle.format(_Parse_ctx, _Format_ctx); return _Format_ctx.out(); @@ -3606,9 +3606,9 @@ struct _Format_checker { : _Parse_context(_Fmt, _Num_args, _Arg_type), _Parse_funcs{&_Compile_time_parse_format_specs<_Args, _ParseContext>...} {} constexpr void _On_text(const _CharT*, const _CharT*) const noexcept {} - constexpr void _On_replacement_field(const size_t _Id, const _CharT*) const { - _ParseContext _Parse_ctx({}); - (void) _Parse_funcs[_Id](_Parse_ctx); + constexpr void _On_replacement_field(const size_t _Id, const _CharT*) { + _Parse_context.advance_to(_Parse_context.end()); + (void) _Parse_funcs[_Id](_Parse_context); } constexpr const _CharT* _On_format_specs(const size_t _Id, const _CharT* _First, const _CharT*) { _Parse_context.advance_to(_Parse_context.begin() + (_First - _Parse_context.begin()._Unwrapped())); @@ -3643,8 +3643,12 @@ struct _Format_handler { void _On_replacement_field(const size_t _Id, const _CharT*) { auto _Arg = _Get_arg(_Ctx, _Id); - _Ctx.advance_to(_STD visit_format_arg( - _Default_arg_formatter<_OutputIt, _CharT>{_Ctx.out(), _Ctx._Get_args(), _Ctx._Get_lazy_locale()}, _Arg)); + if (_Arg._Active_state == _Basic_format_arg_type::_Custom_type) { + _Parse_context.advance_to(_Parse_context.end()); + } + _Ctx.advance_to(_STD visit_format_arg(_Default_arg_formatter<_OutputIt, _CharT>{_Ctx.out(), _Ctx._Get_args(), + _Ctx._Get_lazy_locale(), _Parse_context}, + _Arg)); } const _CharT* _On_format_specs(const size_t _Id, const _CharT* _First, const _CharT* _Last) { diff --git a/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp b/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp index 999ab832d81..54310229b2c 100644 --- a/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp +++ b/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp @@ -276,6 +276,65 @@ void test_basic_format_context_construction() { static_assert(!is_constructible_with_trailing_empty_brace_impl&>); } +// Test GH-4636 ": Call to next_arg_id may result in unexpected error (regression)" + +struct FormatNextArg {}; + +template +struct std::formatter { +public: + template + constexpr auto parse(ParseContext& ctx) { + auto it = ctx.begin(); + if (it != ctx.end() && *it != '}') { + throw std::format_error{"Expected empty spec"}; + } + + arg_id = ctx.next_arg_id(); + return it; + } + + template + auto format(FormatNextArg, FormatContext& ctx) const { + return std::format_to(ctx.out(), TYPED_LITERAL(CharT, "arg-id: {}"), arg_id); + } + +private: + size_t arg_id; +}; + +template +void test_parsing_with_next_id() { + assert(format(TYPED_LITERAL(CharT, "{}, {}"), FormatNextArg{}, 0, FormatNextArg{}, TYPED_LITERAL(CharT, "1")) + == TYPED_LITERAL(CharT, "arg-id: 1, arg-id: 3")); + assert(format(TYPED_LITERAL(CharT, "{:}, {:}"), FormatNextArg{}, 2, FormatNextArg{}, TYPED_LITERAL(CharT, "3")) + == TYPED_LITERAL(CharT, "arg-id: 1, arg-id: 3")); +} + +struct NeedMagicWord {}; + +template +struct std::formatter { + constexpr auto parse(basic_format_parse_context const& ctx) { + constexpr basic_string_view magic_word{TYPED_LITERAL(CharT, "narf")}; + auto [i, j] = ranges::mismatch(ctx, magic_word); + if (j != magic_word.end()) { + throw runtime_error{"you didn't say the magic word!"}; + } + return i; + } + + template + auto format(NeedMagicWord, basic_format_context& ctx) const { + return ctx.out(); + } +}; + +template +void test_parsing_needing_magic_word() { + assert(format(TYPED_LITERAL(CharT, "{:narf}"), NeedMagicWord{}).empty()); +} + int main() { test_format_family_overloads(); test_format_family_overloads(); @@ -297,5 +356,9 @@ int main() { test_basic_format_context_construction(); test_basic_format_context_construction(); test_basic_format_context_construction, wchar_t>(); - return 0; + + test_parsing_with_next_id(); + test_parsing_with_next_id(); + test_parsing_needing_magic_word(); + test_parsing_needing_magic_word(); } From a1145133aa83124f1f08f49aac843613ec84419e Mon Sep 17 00:00:00 2001 From: "A. Jiang" Date: Tue, 30 Apr 2024 07:41:27 +0800 Subject: [PATCH 2/5] Try to fix `_On_replacement_field` again And fix some `parse` functions in the test. --- stl/inc/format | 8 ++++---- .../test.cpp | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/stl/inc/format b/stl/inc/format index 7e2e9623839..659734c7bd6 100644 --- a/stl/inc/format +++ b/stl/inc/format @@ -3606,8 +3606,8 @@ struct _Format_checker { : _Parse_context(_Fmt, _Num_args, _Arg_type), _Parse_funcs{&_Compile_time_parse_format_specs<_Args, _ParseContext>...} {} constexpr void _On_text(const _CharT*, const _CharT*) const noexcept {} - constexpr void _On_replacement_field(const size_t _Id, const _CharT*) { - _Parse_context.advance_to(_Parse_context.end()); + constexpr void _On_replacement_field(const size_t _Id, const _CharT* _Last) { + _Parse_context.advance_to(_Parse_context.begin() + (_Last - &*_Parse_context.begin())); (void) _Parse_funcs[_Id](_Parse_context); } constexpr const _CharT* _On_format_specs(const size_t _Id, const _CharT* _First, const _CharT*) { @@ -3641,10 +3641,10 @@ struct _Format_handler { _Ctx.advance_to(_RANGES _Copy_unchecked(_First, _Last, _Ctx.out()).out); } - void _On_replacement_field(const size_t _Id, const _CharT*) { + void _On_replacement_field(const size_t _Id, const _CharT* _Last) { auto _Arg = _Get_arg(_Ctx, _Id); if (_Arg._Active_state == _Basic_format_arg_type::_Custom_type) { - _Parse_context.advance_to(_Parse_context.end()); + _Parse_context.advance_to(_Parse_context.begin() + (_Last - &*_Parse_context.begin())); } _Ctx.advance_to(_STD visit_format_arg(_Default_arg_formatter<_OutputIt, _CharT>{_Ctx.out(), _Ctx._Get_args(), _Ctx._Get_lazy_locale(), _Parse_context}, diff --git a/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp b/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp index 54310229b2c..74815fc4068 100644 --- a/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp +++ b/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp @@ -58,10 +58,10 @@ struct not_const_formattable_type { template <> struct std::formatter { constexpr basic_format_parse_context::iterator parse(basic_format_parse_context& parse_ctx) { - if (parse_ctx.begin() != parse_ctx.end()) { + if (parse_ctx.begin() != parse_ctx.end() && *parse_ctx.begin() != '}') { throw format_error{"only empty specs please"}; } - return parse_ctx.end(); + return parse_ctx.begin(); } format_context::iterator format(const basic_custom_formattable_type& val, format_context& ctx) const { ctx.advance_to(copy(val.string_content.begin(), val.string_content.end(), ctx.out())); @@ -72,10 +72,10 @@ struct std::formatter { template <> struct std::formatter { constexpr basic_format_parse_context::iterator parse(basic_format_parse_context& parse_ctx) { - if (parse_ctx.begin() != parse_ctx.end()) { + if (parse_ctx.begin() != parse_ctx.end() && *parse_ctx.begin() != '}') { throw format_error{"only empty specs please"}; } - return parse_ctx.end(); + return parse_ctx.begin(); } format_context::iterator format(not_const_formattable_type& val, format_context& ctx) const { ctx.advance_to(copy(val.string_content.begin(), val.string_content.end(), ctx.out())); @@ -287,7 +287,7 @@ struct std::formatter { constexpr auto parse(ParseContext& ctx) { auto it = ctx.begin(); if (it != ctx.end() && *it != '}') { - throw std::format_error{"Expected empty spec"}; + throw format_error{"Expected empty spec"}; } arg_id = ctx.next_arg_id(); @@ -296,7 +296,7 @@ struct std::formatter { template auto format(FormatNextArg, FormatContext& ctx) const { - return std::format_to(ctx.out(), TYPED_LITERAL(CharT, "arg-id: {}"), arg_id); + return format_to(ctx.out(), TYPED_LITERAL(CharT, "arg-id: {}"), arg_id); } private: From ab24ba42772314ebcd186a5bbfff586f2cac0d3f Mon Sep 17 00:00:00 2001 From: "A. Jiang" Date: Tue, 30 Apr 2024 14:19:37 +0800 Subject: [PATCH 3/5] Fix a `formatter` specialization in the test --- .../P0645R10_text_formatting_custom_formatting/test.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp b/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp index 74815fc4068..9439ed955ab 100644 --- a/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp +++ b/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp @@ -319,7 +319,10 @@ struct std::formatter { constexpr basic_string_view magic_word{TYPED_LITERAL(CharT, "narf")}; auto [i, j] = ranges::mismatch(ctx, magic_word); if (j != magic_word.end()) { - throw runtime_error{"you didn't say the magic word!"}; + throw format_error{"you didn't say the magic word!"}; + } + if (i != ctx.end() && *i != '}') { + throw format_error{"the whole spec must be the magic word!"}; } return i; } From 092693eb69647d0fa7b8ea9b5ec15731ca0597e7 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Wed, 15 May 2024 22:51:54 -0700 Subject: [PATCH 4/5] `&*_Parse_context.begin()` => `_Parse_context.begin()._Unwrapped()` --- stl/inc/format | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/stl/inc/format b/stl/inc/format index 659734c7bd6..95df0a427d8 100644 --- a/stl/inc/format +++ b/stl/inc/format @@ -3607,7 +3607,7 @@ struct _Format_checker { _Parse_funcs{&_Compile_time_parse_format_specs<_Args, _ParseContext>...} {} constexpr void _On_text(const _CharT*, const _CharT*) const noexcept {} constexpr void _On_replacement_field(const size_t _Id, const _CharT* _Last) { - _Parse_context.advance_to(_Parse_context.begin() + (_Last - &*_Parse_context.begin())); + _Parse_context.advance_to(_Parse_context.begin() + (_Last - _Parse_context.begin()._Unwrapped())); (void) _Parse_funcs[_Id](_Parse_context); } constexpr const _CharT* _On_format_specs(const size_t _Id, const _CharT* _First, const _CharT*) { @@ -3644,7 +3644,7 @@ struct _Format_handler { void _On_replacement_field(const size_t _Id, const _CharT* _Last) { auto _Arg = _Get_arg(_Ctx, _Id); if (_Arg._Active_state == _Basic_format_arg_type::_Custom_type) { - _Parse_context.advance_to(_Parse_context.begin() + (_Last - &*_Parse_context.begin())); + _Parse_context.advance_to(_Parse_context.begin() + (_Last - _Parse_context.begin()._Unwrapped())); } _Ctx.advance_to(_STD visit_format_arg(_Default_arg_formatter<_OutputIt, _CharT>{_Ctx.out(), _Ctx._Get_args(), _Ctx._Get_lazy_locale(), _Parse_context}, @@ -3652,7 +3652,7 @@ struct _Format_handler { } const _CharT* _On_format_specs(const size_t _Id, const _CharT* _First, const _CharT* _Last) { - _Parse_context.advance_to(_Parse_context.begin() + (_First - &*_Parse_context.begin())); + _Parse_context.advance_to(_Parse_context.begin() + (_First - _Parse_context.begin()._Unwrapped())); auto _Arg = _Get_arg(_Ctx, _Id); if (_Arg._Active_state == _Basic_format_arg_type::_Custom_type) { _Arg._Custom_state.format(_Parse_context, _Ctx); From 8cad6d5f9d1f9e2030b73ec5323b50008e85952e Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Wed, 15 May 2024 22:52:57 -0700 Subject: [PATCH 5/5] Minor nitpicks. --- .../tests/P0645R10_text_formatting_custom_formatting/test.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp b/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp index 9439ed955ab..6a9db58b132 100644 --- a/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp +++ b/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -315,12 +316,13 @@ struct NeedMagicWord {}; template struct std::formatter { - constexpr auto parse(basic_format_parse_context const& ctx) { + constexpr auto parse(const basic_format_parse_context& ctx) { constexpr basic_string_view magic_word{TYPED_LITERAL(CharT, "narf")}; auto [i, j] = ranges::mismatch(ctx, magic_word); if (j != magic_word.end()) { throw format_error{"you didn't say the magic word!"}; } + if (i != ctx.end() && *i != '}') { throw format_error{"the whole spec must be the magic word!"}; }