Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
Variadic print#5829
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Variadic print#5829
Uh oh!
There was an error while loading.Please reload this page.
Conversation
matthijskooijman left a comment
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.
Overall, these changes look good. I've only looked at the AVR changes, I assume the SAM changes are identical?
Looking at Print.cpp now, I think that it would be beneficial to also move them other single-lineprint() methods to Print.h and make themalways_inline (or perhaps justinline). Or is there any reason not to?
What I am wondering is how LTO plays into this. IIRC we have that enabled by default now, so you would think the compiler would have been able to do this inlining already. Perhaps it decides not to, so only thealways_inline attributes are needed. Would you care to try adding just the attribute, without moving the definitions from Print.cpp to Print.h and see if that works?
Regarding the commit structure: You have this weird PR merging commit included, I suspect you did something weird to update your fork of the repo or something? That commit should go.
The actual changes are all lumped together in the second commit, but I think there are a few logical changes here that should ideally be in separate commits. I think it's three changes: Replacingprintln with a template, addingalways_inline & moving definitions, and adding varadic versions.
| #defineOCT8 | ||
| #defineBIN2 | ||
| #defineINLINE__attribute__ ((__always_inline__)) inline// undefined at end |
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 wonder if this should be calledPRINT_INLINE or something else more explicit (ARDUINO_INLINE?), sinceINLINE is more_likely to conflict with other header files. The fact that you undef it later helps, but not if another definition ofINLINE is included beforePrint.h.
I also believe theinline keyword should not be needed here, since these are all methods. Doesn't really hurt either, though.
| /** Variadic methods **/ | ||
| #if __cplusplus >= 201103L | ||
| // Ensure there are at least two parameters to avoid infinite recursion. |
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.
This deserves a bit more explanation, best to include an example.
e59f96d tocf5de58CompareRemove all definitions of println(...) from Print.cpp except println(void),and replace them with inline methods in Print.h.This simplifies the code in Print.cpp improving maintainability.
Remove print(char | char[] | Printable | unsigned char | int | unsigned int)from Print.cpp and replace them with inline versions in Print.h.
Add explicit print() support for signed char, unsigned/signed short, and float.Also, add `signed` keyword explicitly to int and long (even if unnecessary).Notice that `char` and `signed char` are considered different types,even if their ranges are the same (ditto for `unsigned char` in SAM).Both AVR and SAM seem to define [u]int8_t explicitly as [un]signed charand not as char, so printing them will print them as numbers as expected.(This does not apply to `int` and `signed int`; those are the same type.)
Add GCC __attribute__ ((__always_inline__)) to inlined methods in Print.h.This seems necessary since not doing so increases the executable size(probably because that would create several function definitions),even with LTO optimizations.This change combined with the inlining of print/printf methods seems to reducethe executable size for most applications by a fair amount.(Note that the `inline` keyword does not imply actual inlining.)
Allows combining multiple arguments into a single call to Print::print[ln], like Serial.print("Answer=", 66, HEX).This feature requires C++11 or newer to work, but if it is not available the old calls can still be used (the change has been wrapped in `#if __cplusplus >= 201103L ... #endif`).This change is backwards compatible: print(42, 16) prints "2A" as always, not "4216" (the latter can still be achieved with print(42, "", 16) ). This also works with multiple arguments: any numeric argument followed by an int will be treated as a (number, format) pair.The variadic function calls are inlined using an __attribute__ ((__always_inline__)), so they're equivalent to writing multiple sequential calls to print with one argument.cf5de58 to587eec5Comparecousteaulecommandant commentedJan 10, 2017
Everything done, thanks for the feedback! As requested via IRC, here's anexample sketch for testing backwards compatibility: As can be seen, the resulting size is slightly smaller with the new code. However, some examples show ahuge improvement, such as this one, where using inlined functions implies creating only 3 functions and not 4: Regarding documentation, the only feature that has been added is the variadic support (the other commits inside this PR are just optimizations or improvements on readability and maintainability), so there shouldn't be big changes in the documentation. In any case, the current documentation is still valid since these changes were designed to be backwards compatible. |
matthijskooijman commentedJun 19, 2017
@cousteaulecommandant, I finally got around to having another look at this. As we discussed on IRC, I'd like to see how feasible it is to extend this to also support custom formatters and/or more formatting options. As a proof-of-concept I madematthijskooijman@a3f0e33 today, which allows passing custom "formatter" objects after a value to format (e.g. instead of I'm still not sure how to add formattingoptions (e.g. options to default or other formatters) and especially how to combine these, so that needs a bit more thought. |
cousteaulecommandant commentedJun 21, 2017
Personally I don't think we should be putting too much effort into formatters for
In other words, if complex and extensible formatters end up being added, I'd go for functions that return a ( |
| // Ensure there are at least two parameters to avoid infinite recursion. | ||
| // e.g. `StringSumHelper s; print(s)` may be treated as `print(s, ...)` | ||
| // with `...` being the empty list, thus calling `print(s)` again. | ||
| // (This is because print(StringSumHelper) isn't explicitly defined.) |
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 don't agree with this - with one parameter, won't this simply fall back onprint(), which you could define privately as a no-op?
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 recall that this was tried too, but that triggered other corner cases (that I can't recall right now). With care, it can be made to work though (I have some local changes on top of this PR that do so, as well as add some formatting support, but I can't get around to finishing them...).
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.
with one parameter, won't this simply fall back on
print(), which you could define privately as a no-op?
It will fall back onprint(s) followed byprint(). The latter could indeed be implemented as a no-op, but the former will be expanded toprint(s) followed byprint() again, entering an infinite loop (at compile time).
I don't remember the exact situation that caused this (I think it was something about passing aStringSumHelper but I cannot come up with an example right now); only that infinite recursion happened.
CLAassistant commentedApr 9, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
Allows combiningmultiple arguments into a single call to Print::print[ln], like Serial.print("Answer=", 66, HEX).
This feature requires C++11 or newer to work, but if it is not available the old calls can still be used.
This change isbackwards compatible: print(42, 16) prints "2A" as always, not "4216" (the latter can still be achieved with print(42, "", 16) ).
Many functions from Print.cpp have been removed and replaced by inline versions in Print.h, most notably the println(...) ones. This results in a simplified (and thus easier to maintain) Print.cpp and slightly smaller executables.