2. is probably yet another argument to switch official APIs to use e.g.int32_t instead oflong, see alsoDocumentation should use more specific types Arduino#4525 for related discussion.

@matthijskooijman: The overflow affects a local variable, not exposed to the API.

1. is probably yet another argument to switch official APIs to use e.g.int32_t instead oflong, see alsoarduino/Arduino#4525 for related discussion.

While I technically agree with@matthijskooijman I've just yesterday participated in a discussion with@tigoe in which he expressed his dismay on some prototype code usinguint8_t (instead ofbyte) and general usage ofuintXX_t types (Hisopinion voiced in 2016 seems to be constant over time 😉 ). Let me also add that this my last word on this topicwithin this PR, if you feel it's something we should discuss in more depth please open a new issue within this repository.

@edgar-bonet Thank you very much for providing this first step on fixing your issue (A failing test is a great was to start!). Considering floating point comparison you can use theCatch2 providedApprox method, e.g.

REQUIRE(mock.parseFloat() == Approx(-3.141592654f);

Here you can find more documentation how to perform such floating point comparisons.

@aentinger: Thanks for the info! I did not know about this library. TheApprox() macro definitely makes both the test code and the error message more readable. I just pushed a commit to use this macro.

Now, I have a question for you.

If the stream provides an integer larger thanLONG_MAX but smaller thanFLT_MAX, the variablevalue also overflows. Whereas this is not exactly the same issue as reading too many decimal digits after the radix point, it is strongly related, and its fix involves the same lines of code as the current issue. My question is: should this “parse large integer” problem be considered another aspect of this issue, or should we handle it as a separate issue?

In the first case, I would add the relevant test to this PR, and then submit a PR than handles both situations. In the second case, I would wait for this issue to be sorted out before moving to the “parse large integer” problem.

Thank you integratingApprox, this indeed makes the test code so much easier. I'd say theparseInt should be handled separately, could you please also create an issue and the smallest failing test?

@matthijskooijman: The overflow affects a local variable, not exposed to the API.

Right, that would make it even easier to change fromlong toint32_t. IMHO that would be good to change, since it makes the code behave more consistent across platforms. And since it's already 32-bits, I don't think any of the "use int because that's usually fastest on the current platform" arguments apply here. But this is a little off-topic for this PR, maybe.

@aentinger wrote:

I'd say theparseInt should be handled separately

Sorry, I was not clear enough. It is not aboutparseInt. The issue isparseFloat overflowing a local variable when it reads too many digits. And it can be split into two sub-issues:

1. Too many digitsafter the decimal point → can be fixed by ignoring the extra digits.

2. Too many digitsbefore the decimal point (e.g. a large integer) → the extra digits cannot simply be ignored.

My question is whether these two points should be considered parts of the same issue or separate issues.

@matthijskooijman: I do think it is off-topic.

Good morning@edgar-bonet 👋 ☕
Thank you very much for clarifying this point 👍 Since those issues also concern issues withinparseFloat in combination with a large number of digits I think it's okay to handle them within this PR. Could you please add additional failing tests for both too many digits after and before the decimal point?

Hi@aentinger, thanks for your answer.

I just added a failing test case with a number that has too many digits before the decimal point. This is in addition to the previous one that has too many digits after the decimal point.

Should I squash the three commits together and force-push?

Should I squash the three commits together and force-push?

Please no. I already added a first possible solution on how to address this issue. Please to a pull first before your next commit or you've got disentangle it with the commit I pushed 😉

Codecov Report

Merging#133 (6197511) intomaster (78f3f41) willdecrease coverage by0.34%.
The diff coverage is100.00%.

@@            Coverage Diff             @@##           master     #133      +/-   ##==========================================- Coverage   96.41%   96.06%   -0.35%==========================================  Files          14       14                Lines         837      839       +2     ==========================================- Hits          807      806       -1- Misses         30       33       +3

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update78f3f41...6197511. Read thecomment docs.

I had considered parsing straight into a float, but I then assumed the original code used an integer for performance reasons. My tests show parsing into a float takes about 50% more processing time an AVR.

My tests show parsing into a float takes about 50% more processing time an AVR.

Right nowArduinoCore-API is not used onArduinoCore-avr. There are plans for doing so but the timeline is more than a bit fuzzy. Also, as far as I can see all future Arduino platforms will be based on 32-Bit ARM MCUs or comuting-equivalent MCUs.

EDIT:ArduinoCore-API is used withArduinoCore-megaavr so there is a 8-Bit AVR core which will be affected by this change.

As requested, I just pushed my proposed change. Prior to this, I took the liberty to modify the test “A float is provided with too many digits after the decimal point” by adding more digits, in order to evidence the issue the last commit is fixing.