-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fixed issue with formatting to an array of chars #1171
Conversation
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.
Thanks for the PR! Mostly looks good, just two comments inline.
include/fmt/prepare.h
Outdated
@@ -246,14 +246,19 @@ class prepared_format { | |||
} | |||
|
|||
private: | |||
typedef typename buffer_context<char_type>::type context; | |||
template <typename Range, typename Context, typename... StoreArgs> | |||
auto vformat_to(Range out, format_arg_store<Context, StoreArgs...> args) const |
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.
vformat*
functions should take basic_format_args
not format_arg_store
.
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.
Those are private methods. I've introduced the overload that takes format_arg_store
, because calling prepared_format::format_to()
with char*
as an output iterator, introduced compilation error could not match 'basic_format_args' against 'format_arg_store'
. That's why I decided to create the overload, explicitly create basic_format_args
there and pass it to the proper vformat
implementation.
Does it resolve your doubts or it's still wrong? If it's wrong, what do you propose?
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.
The matching error is because template argument deduction doesn't play well with implicit conversion from format_arg_store
to format_args
. I suggest removing the extra overload and adding braces to force the conversion as it is done in
Line 1357 in 87fbc6f
{internal::make_args_checked(format_str, args...)}); |
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.
Hmm, I think that's not exactly the case. Braces help here because with such call, compiler doesn't need to deduce context
for basic_format_args
, thus whole basic_format_args<context>
type is known. It's known because Char
type known, as well.
Line 1317 in 87fbc6f
basic_format_args<buffer_context<Char>> args) { |
In my case, the context
type is not known and needs to be deduced, so basic_format_args
is not known and can not be deduced because compiler would need to perform implicit conversion.
To handle this, I've added explicit call to basic_format_args
ctors. Please check it and let me know if it looks reasonable.
c479b5f
to
aa68b28
Compare
aa68b28
to
b997955
Compare
Thanks! |
It should fix #1169. Added a couple of tests to prevent such issues in the future. Please, let me know if I forgot about any
prepared_format::format_to
use case.