Variadic print

[cousteaulecommandant]

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.
This change is backwards 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.

[matthijskooijman]

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-line print() methods to Print.h and make them always_inline (or perhaps just inline). 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 the always_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: Replacing println with a template, adding always_inline & moving definitions, and adding varadic versions.

[matthijskooijman]

I wonder if this should be called PRINT_INLINE or something else more explicit (ARDUINO_INLINE?), since INLINE is more_likely to conflict with other header files. The fact that you undef it later helps, but not if another definition of INLINE is included before Print.h.

I also believe the inline keyword should not be needed here, since these are all methods. Doesn't really hurt either, though.

[matthijskooijman]

This deserves a bit more explanation, best to include an example.

[cousteaulecommandant]

Everything done, thanks for the feedback!

As requested via IRC, here's an example sketch for testing backwards compatibility:
https://gist.github.com/cousteaulecommandant/3083c33cc83497d05e1fa12ae8cfaf17#file-example_backwards_compat-ino

As can be seen, the resulting size is slightly smaller with the new code. However, some examples show a huge improvement, such as this one, where using inlined functions implies creating only 3 functions and not 4:
https://gist.github.com/cousteaulecommandant/3083c33cc83497d05e1fa12ae8cfaf17#file-example_println_inlining-ino
On this other example, however, the resulting code is a few bytes larger, since it has to include several extra calls to println(void) that were formerly included in the previous call:
https://gist.github.com/cousteaulecommandant/3083c33cc83497d05e1fa12ae8cfaf17#file-example_println_inlining_2-ino

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]

@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 made matthijskooijman@a3f0e33 today, which allows passing custom "formatter" objects after a value to format (e.g. instead of HEX). I've found that to reliably do this, I needed to move the actual printing code into a (non-variadic) doPrint() method, so print() can just take care of the variadic arguments (using some template magic in check_type, in a way similar to std::enable_if, to decide whether a second argument is a formatter or just something to print). As an added advantage, it seems it is no longer needed to repeat the list of int-as-second-argument-for-base-formatting as variadic versions). It might make sense to move this change into your PR if it really ends up being needed.

I'm still not sure how to add formatting options (e.g. options to default or other formatters) and especially how to combine these, so that needs a bit more thought.

[cousteaulecommandant]

Personally I don't think we should be putting too much effort into formatters for Print, since

• Print is already quite complex,
• something like Serial.print(Format(number, digits, base, etc)) would be easier to implement and understand, and (depending on how it's implemented) not only useful for Print but also String and other uses,
• printf (Add function "printf" to AVR core class "print" #5938) might be a simpler solution for complex formatting (and if "%02X" is too scary for newbies, maybe a "printf formatter constructor class" could be developed).

In other words, if complex and extensible formatters end up being added, I'd go for functions that return a Printable or a String rather than arguments to Print::print, as they would be much easier to implement and document, and move the formatting problem somewhere else.

(Serial.print(number, HEX) et al would need to be kept for backwards compatibility, but I would avoid going in that direction for new formatting features.)

[eric-wieser]

I don't agree with this - with one parameter, won't this simply fall back on print(), which you could define privately as a no-op?

[matthijskooijman]

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...).

[cousteaulecommandant]

with one parameter, won't this simply fall back on print(), which you could define privately as a no-op?

It will fall back on print(s) followed by print(). The latter could indeed be implemented as a no-op, but the former will be expanded to print(s) followed by print() 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 a StringSumHelper but I cannot come up with an example right now); only that infinite recursion happened.

[CLAassistant]

All committers have signed the CLA.