Prevent fast_float::from_chars from parsing whitespaces and leading '+'…

[eugenegff]

Prevent fast_float::from_chars from parsing whitespaces and leading '+'sign, similar to MSVC and integer LLVM std::from_chars behavior. See C++17 20.19.3.(7.1) and http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0067r5.html

This pull request at the first glance might seems to be unreasonable - char sequences with clear meaning became non-parseable, even if they still could be parsed by strtod(). But C++17 made this decision deliberately, to increase parsing efficiency. In sutuations when whitespaces are expected - they could be skipped prior to the from_chars() invocation, without requiring everyone to pay for the feature that is not required for everyone. Similarly leading plus sign could be skipped prior to the from_chars() invocation if the next char is not '-' and from_chars does not silently skip whitespaces - easy enough.

Note, that both MSVC and (currently integer only) LLVM implementations of std::from_chars() does not parse leading whitespaces nor leading '+' sign.

While allowing fast_float::from_chars() to parse leading whitespaces and '+' might seems as improvement - it actually creates non-standard dialect of 'from_chars', and code that uses fast_float::from_chars() would not be portable to std::from_chars() from C++17.

[lemire]

That sounds very thoughtful.

Of course, our tests are currently failing.

[lemire]

@eugenegff Have you had some luck running the tests?

[eugenegff]

I think search and replace ["+] => ["] will fix everything, as I don't see leading spaces. I'll try it.

[eugenegff]

Tests are passed now.

[lemire]

Ok. At this point I will invite all users of this library to chime in. This is going to be a breaking change. It is fine but I want everyone to be aware. (e.g., @timkpaine)

And @biojppm can you have a look ?

[eugenegff]

Interestingly, the nuance that from_chars should not parse whitespaces is discussed in http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0067r5.html, implemented by both MSVC and LLVM, documented in MSVC documentation https://docs.microsoft.com/en-us/cpp/standard-library/charconv-functions?view=msvc-160 - but is not reflected in standard wording proposed by http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0067r5.html and accepted into the C++17 standard.

[eugenegff]

Abseil absl::from_chars() does not parse leading whitespaces and plus sign too. https://github.com/abseil/abseil-cpp/blob/master/absl/strings/charconv.cc , look at FromCharsImpl

[lemire]

@eugenegff I am not arguing and we will assuredly merge this but I would like to give time to users of this library to chime in. There might be nuances that we are not seeing.

Note that it will just make the library faster. I am all for that !!! More speed.
⚡ ⚡ ⚡ ⚡ ⚡ ⚡ ⚡
⚡ ⚡ ⚡ ⚡ ⚡ ⚡ ⚡

[lemire]

@eugenegff And it is a fantastic pull request, to be clear. I don't want to sound negative. I am just cautious.

[lemire]

@biojppm @nealrichardson @timkpaine @alexey-milovidov @kitaisreal

This is an API breaking change (which I support) but I'd like for people who rely on the library to chime in so we can anticipate problems. I am very preoccupied about breaking people's code, so please speak up if you have concerns.

[pitrou]

Thanks for the heads-up. This would be fine for us (Arrow).

[alexey-milovidov]

Thank you!
More speed is always alright :)

[lemire]

Let us merge and release then.

[biojppm]

Very late to the party, but also eager to express support for the idea.