-
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
Allow %s as generic format specifier in printf #453
Conversation
Signed integers are formatted as %d Unsigned integers are formatted as %u Doubles are formatted as %f Chars are formatted as %c Void Pointers are formatted as %p
For #449 |
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 working on this, just two comments re implementation.
@@ -92,9 +92,18 @@ class ArgConverter : public ArgVisitor<ArgConverter<T>, void> { | |||
visit_any_int(value); | |||
} | |||
|
|||
void visit_char(char value) { | |||
if (type_ != 's' && type_ != 'S') |
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 think that having uppercase S
is redundant and inconsistent with other specifiers. Please leave only s
here and below.
// all floating point types | ||
spec.type_ = 'f'; | ||
} | ||
} |
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.
Please convert this if-else chain into a visitor and merge with the similar logic under for integers right below.
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.
A few more comments.
void visit_char(char value) { | ||
if (type_ != 's') | ||
visit_any_int(value); | ||
} |
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.
Isn't this handled by DefaultType
?
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.
ArgConverter runs before DefaultType, and I couldn't see a clean way to switch it around.
wchar_t visit_any_int(T) { return 'd'; } | ||
|
||
template <typename T> | ||
wchar_t visit_any_double(T) { return 'f'; } |
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 suggest using g
here because it is the default floating-point format.
template <typename U> | ||
void visit_any_int(U value) { | ||
bool is_signed = type_ == 'd' || type_ == 'i'; | ||
if (type_ == 's') { | ||
is_signed = std::numeric_limits<U>::is_signed; | ||
} |
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.
Same here.
|
||
if (spec.type_ == 's') { | ||
// set the format type to the default if 's' is specified | ||
spec.type_ = internal::DefaultType().visit(arg); |
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.
nit: please use 2 space indent
Merged, thanks! |
Thankyou :) |
@mojoBrendan @vitaut #504 reported warnings because the return type of your |
Nevermind, it was done in PR #502. |
Signed integers are formatted as %d
Unsigned integers are formatted as %u
Doubles are formatted as %f
Chars are formatted as %c
Void Pointers are formatted as %p