ArduinoCore-API icon indicating copy to clipboard operation
ArduinoCore-API copied to clipboard

Added stream insertion operator.

Open jfjlaros opened this issue 2 years ago • 17 comments

In this PR, an implementation of Pair (analogous to std::pair), a helper function to create a Pair and two Stream insertion operators are added.

The basic Stream insertion operator allows for compact and convenient writing to a Stream. An overload is provided to allow for modifiers like BIN, HEX and the number of digits.

Usage:

int a {9};
int b {10};
Serial << "You have " << a << " out of " << b << " retries left.\r\n";
// Prints: "You have 9 out of 10 retries left."
Serial << makePair(1.2, 4) << ' ' << makePair(12, BIN) << "\r\n";
// Prints: "1.2000 1100"

This fixes #180.

jfjlaros avatar Feb 06 '23 21:02 jfjlaros

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (844e4bf) 95.70% compared to head (0578e7e) 95.57%. Report is 76 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #181      +/-   ##
==========================================
- Coverage   95.70%   95.57%   -0.14%     
==========================================
  Files          13       16       +3     
  Lines         954     1084     +130     
==========================================
+ Hits          913     1036     +123     
- Misses         41       48       +7     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Feb 06 '23 21:02 codecov-commenter

Hi @jfjlaros :coffee: :wave:

Thank you for your contribution. It should still be comparatively easy to add test code within the test subfolder.

Frankly, I'm not going to approve the API in its current state, makePair just sounds way to similar to std::pair, but has a completely different effect. That's going to throw people for sure.

aentinger avatar Feb 07 '23 04:02 aentinger

I will have a look at the unit tests.

makePair just sounds way to similar to std::pair

Hmm. makePair is supposed to be analogous to std::make_pair, the container is named Pair (analogous to std::pair).

I could add a second constructor to Pair to allow for syntax like this:

Serial << Pair(1.2, 4) << ' ' << Pair(12, BIN) << "\r\n";

then the function makePair could be dropped if that is what you prefer.

jfjlaros avatar Feb 07 '23 07:02 jfjlaros

Hmm. makePair is supposed to be analogous to std::make_pair, the container is named Pair (analogous to std::pair).

Well yes. So why re-invent std::pair? Maybe use format or similar. Something that tells the user what's about to happen.

aentinger avatar Feb 07 '23 09:02 aentinger

why re-invent std::pair?

Because the standard template library is not available, or is this not the case for cores that depend on this repository?

I could make something more specialised I guess.

jfjlaros avatar Feb 07 '23 11:02 jfjlaros

The unit tests seem to fail because of a failed upload in the workflow. Perhaps someone could re-trigger it.

jfjlaros avatar Feb 07 '23 15:02 jfjlaros

Thanks @per1234.

@aentinger, I followed your suggestions and updated the PR. I guess now the description should be as follows.


In this PR, the following is added:

  • Format: a container for one arbitrary object and one modifier of type int.
  • format(): a helper function to create a Format instance.
  • << a basic Stream insertion operator for compact and convenient writing to a Stream.
  • << an overload to allow for modifiers like BIN, HEX and the number of digits to be written.

Usage:

int a {9};
int b {10};
Serial << "You have " << a << " out of " << b << " retries left.\r\n";
// Prints: "You have 9 out of 10 retries left."
Serial << format(1.2, 4) << ' ' << format(12, BIN) << "\r\n";
// Prints: "1.2000 1100"

jfjlaros avatar Feb 07 '23 17:02 jfjlaros

Hmm, same problem with the failed upload again...

jfjlaros avatar Feb 07 '23 20:02 jfjlaros

I apologize for the inconvenience @jfjlaros. Unfortunately these spurious failures caused by transient network faults are a common occurrence when using GitHub Actions.

I reran the job and it passed this time.

per1234 avatar Feb 07 '23 20:02 per1234

Because the standard template library is not available, or is this not the case for cores that depend on this repository?

std::pair is available on some Arduino platforms and by creating something so similar, you violate the expectations of all developers when you create something very similarly named but doing something different. This shall be the last time I discuss this, and you've already adjusted as I can see.

aentinger avatar Feb 08 '23 04:02 aentinger

Because the standard template library is not available, or is this not the case for cores that depend on this repository?

I'm aware that at least AVR does not support it, I think most if not all other cores do have it. For AVR, this might be solveable by enabling libstdc++, see https://github.com/arduino/toolchain-avr/issues/89 about this (this is off-topic for this PR, but I wanted to mention it here in case it is interesting to anyone here).

matthijskooijman avatar Feb 08 '23 11:02 matthijskooijman

this might be solveable by enabling libstdc++

That would be great, I have my own implementations of std::vector, std::tuple and std::enable_if that I would happily part with.

jfjlaros avatar Feb 08 '23 12:02 jfjlaros

Perhaps the "Changes requested" block can be resolved (as all the underlying requests have been addressed)?

jfjlaros avatar Aug 01 '23 07:08 jfjlaros

Has anyone considered the impact of a global scope function named "format" and a global scope class named "Format" on the existing Arduino ecosystem? Will these cause some programs and libraries to break?

PaulStoffregen avatar Aug 08 '23 14:08 PaulStoffregen

Will these cause some programs and libraries to break?

Probably, I find it hard to tell.

We could simply rename them or not introduce them into the global scope at all. I would prefer the latter.

jfjlaros avatar Aug 08 '23 15:08 jfjlaros

I have removed both the Format class as well as the format function from the global scope. I hope this alleviates any remaining concerns.

jfjlaros avatar Jan 19 '24 19:01 jfjlaros