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

Jump to bottom

Validity check simple replacement fields #4078

Merged
merged 6 commits into from
Nov 7, 2023
Merged

Validity check simple replacement fields #4078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
78002b8
Validity check simple replacement fields
ecatmur Oct 10, 2023
76b6f5b
Update stl/inc/format
ecatmur Oct 10, 2023
b108445
Update format
ecatmur Oct 10, 2023
1a3ec0d
fix tests
ecatmur Oct 13, 2023
2034246
Merge branch 'microsoft:main' into patch-1
ecatmur Oct 15, 2023
b33fd74
Casey's review comment
CaseyCarter Oct 25, 2023
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
5 changes: 4 additions & 1 deletion stl/inc/format
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3519,7 +3519,10 @@ struct _Format_checker {
consteval explicit _Format_checker(basic_string_view<_CharT> _Fmt) noexcept
: _Parse_context(_Fmt, _Num_args), _Parse_funcs{&_Compile_time_parse_format_specs<_Args, _ParseContext>...} {}
constexpr void _On_text(const _CharT*, const _CharT*) const noexcept {}
constexpr void _On_replacement_field(size_t, const _CharT*) const noexcept {}
constexpr void _On_replacement_field(const size_t _Id, const _CharT*) const {
_ParseContext _Parse_ctx({});
Copy link
Member

@CaseyCarter CaseyCarter Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like this isn't conforming, since this parse context is very observably different from what the standard says we will pass to the user's parse function (i.e., to_address(_Parse_ctx.begin()) is not in the user-provided format string). I am not requesting a change, however, since this is consistent with how we handle simple replacement fields when actually formatting - we should maybe address this globally. @barcharcraz what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I think #4640 addresses this.

(void) _Parse_funcs[_Id](_Parse_ctx);
}
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()));
if (_Id < _Num_args) {
Expand Down
4 changes: 2 additions & 2 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 @@ -57,7 +57,7 @@ struct not_const_formattable_type {

template <>
struct std::formatter<basic_custom_formattable_type, char> {
basic_format_parse_context<char>::iterator parse(basic_format_parse_context<char>& parse_ctx) {
constexpr basic_format_parse_context<char>::iterator parse(basic_format_parse_context<char>& parse_ctx) {
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
if (parse_ctx.begin() != parse_ctx.end()) {
throw format_error{"only empty specs please"};
}
Expand All @@ -71,7 +71,7 @@ struct std::formatter<basic_custom_formattable_type, char> {

template <>
struct std::formatter<not_const_formattable_type, char> {
basic_format_parse_context<char>::iterator parse(basic_format_parse_context<char>& parse_ctx) {
constexpr basic_format_parse_context<char>::iterator parse(basic_format_parse_context<char>& parse_ctx) {
if (parse_ctx.begin() != parse_ctx.end()) {
throw format_error{"only empty specs please"};
}
Expand Down