Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<format>: Fix handling of replacement field when format-spec is absent #4640

Merged
merged 5 commits into from
May 20, 2024
Merged

<format>: Fix handling of replacement field when format-spec is absent #4640

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2e64d7f
Fix handling of replacement field when format-spec is absent
frederick-vs-ja Apr 29, 2024
a114513
Try to fix `_On_replacement_field` again
frederick-vs-ja Apr 29, 2024
ab24ba4
Fix a `formatter` specialization in the test
frederick-vs-ja Apr 30, 2024
092693e
`&*_Parse_context.begin()` => `_Parse_context.begin()._Unwrapped()`
StephanTLavavej May 16, 2024
8cad6d5
Minor nitpicks.
StephanTLavavej May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions stl/inc/format
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3537,14 +3537,14 @@ struct _Default_arg_formatter {
_OutputIt _Out;
basic_format_args<_Context> _Args;
_Lazy_locale _Loc;
basic_format_parse_context<_CharT>& _Parse_ctx;

template <class _Ty>
_OutputIt operator()(_Ty _Val) && {
return _Fmt_write<_CharT>(_STD move(_Out), _Val);
}

_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();
Expand Down Expand Up @@ -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* _Last) {
_Parse_context.advance_to(_Parse_context.begin() + (_Last - &*_Parse_context.begin()));
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
(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()));
Expand Down Expand Up @@ -3641,10 +3641,14 @@ 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);
_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.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},
_Arg));
}

const _CharT* _On_format_specs(const size_t _Id, const _CharT* _First, const _CharT* _Last) {
Expand Down
76 changes: 71 additions & 5 deletions tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ struct not_const_formattable_type {
template <>
struct std::formatter<basic_custom_formattable_type, char> {
constexpr basic_format_parse_context<char>::iterator parse(basic_format_parse_context<char>& parse_ctx) {
if (parse_ctx.begin() != parse_ctx.end()) {
if (parse_ctx.begin() != parse_ctx.end() && *parse_ctx.begin() != '}') {
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
throw format_error{"only empty specs please"};
}
return parse_ctx.end();
return parse_ctx.begin();
Comment on lines -64 to +65
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be non-functional, correct? It's a matter of if we "match" the closing '}' or not.

}
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()));
Expand All @@ -72,10 +72,10 @@ struct std::formatter<basic_custom_formattable_type, char> {
template <>
struct std::formatter<not_const_formattable_type, char> {
constexpr basic_format_parse_context<char>::iterator parse(basic_format_parse_context<char>& 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()));
Expand Down Expand Up @@ -276,6 +276,68 @@ void test_basic_format_context_construction() {
static_assert(!is_constructible_with_trailing_empty_brace_impl<context, OutIt, const basic_format_args<context>&>);
}

// Test GH-4636 "<format>: Call to next_arg_id may result in unexpected error (regression)"

struct FormatNextArg {};

template <class CharT>
struct std::formatter<FormatNextArg, CharT> {
public:
template <class ParseContext>
constexpr auto parse(ParseContext& ctx) {
auto it = ctx.begin();
if (it != ctx.end() && *it != '}') {
throw format_error{"Expected empty spec"};
}

arg_id = ctx.next_arg_id();
return it;
}

template <class FormatContext>
auto format(FormatNextArg, FormatContext& ctx) const {
return format_to(ctx.out(), TYPED_LITERAL(CharT, "arg-id: {}"), arg_id);
}

private:
size_t arg_id;
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
};

template <class CharT>
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 <class CharT>
struct std::formatter<NeedMagicWord, CharT> {
constexpr auto parse(basic_format_parse_context<CharT> const& ctx) {
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
constexpr basic_string_view<CharT> 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 != '}') {
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
throw format_error{"the whole spec must be the magic word!"};
}
return i;
}

template <class OutIt>
auto format(NeedMagicWord, basic_format_context<OutIt, CharT>& ctx) const {
return ctx.out();
}
};

template <class CharT>
void test_parsing_needing_magic_word() {
assert(format(TYPED_LITERAL(CharT, "{:narf}"), NeedMagicWord{}).empty());
}

int main() {
test_format_family_overloads<basic_custom_formattable_type>();
test_format_family_overloads<not_const_formattable_type>();
Expand All @@ -297,5 +359,9 @@ int main() {
test_basic_format_context_construction<wchar_t*, wchar_t>();
test_basic_format_context_construction<wstring::iterator, wchar_t>();
test_basic_format_context_construction<back_insert_iterator<wstring>, wchar_t>();
return 0;

test_parsing_with_next_id<char>();
test_parsing_with_next_id<wchar_t>();
test_parsing_needing_magic_word<char>();
test_parsing_needing_magic_word<wchar_t>();
}