-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
Validity check simple replacement fields #4078
Conversation
A custom formatter for a user-defined type might reject omitted format-specs: ```c++ struct A {}; template<> struct std::formatter<A> { constexpr auto parse(std::format_parse_context const& ctx) { std::string_view s("narf"); auto [i, j] = std::mismatch(ctx.begin(), ctx.end(), s.begin(), s.end()); if (j != s.end()) throw std::runtime_error("you didn't say the magic word!"); return i; } auto format(A, std::format_context& ctx) const { return ctx.out(); } }; int main() { std::ignore = std::format("{}", A()); } ``` Per [format.fmt.string]/3 this should be rejected, but MSVC instead accepts and throws at runtime. This implementation seems most likely to not cause problems with existing code; it matches the runtime behavior of _Default_arg_formatter on custom formatters (basic_format_arg<_Context>::handle), so it should accept any format() call that would succeed at runtime.
Co-authored-by: A. Jiang <de34@live.cn>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@@ -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({}); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Thanks for noticing and fixing this problem in |
A custom formatter for a user-defined type might reject omitted (or empty) format-specs:
Per [format.fmt.string]/3 this should be rejected, but MSVC instead accepts and throws at runtime.
This implementation seems most likely to not cause problems with existing code; it matches the runtime behavior of _Default_arg_formatter on custom formatters (basic_format_arg<_Context>::handle), so it should accept any format() call that would succeed at runtime.