search

fastfloat / fast_float

Fast and exact implementation of the C++ from_chars functions for number types: 4x to 10x faster than strtod, part of GCC 12, Chromium, Redis and WebKit/Safari
Apache License 2.0
1.54k stars 132 forks source link

Record parse failure reason and location #252

Closed LeszekSwirski closed 2 months ago

LeszekSwirski commented 3 months ago

In parse_number_string, if there is a parse error, report the specific error as one of the values in a new parse_error enum, and update lastmatch to match the error location. This allows users of the library to print more helpful error messages for invalid inputs.

LeszekSwirski commented 3 months ago

Happy to discuss alternatives to this, this was just the simplest approach that came to mind and I figured it's easier to make a code-based suggestion than an abstract one.

lemire commented 3 months ago

I figured it's easier to make a code-based suggestion than an abstract one.

Yep.

lemire commented 3 months ago

@LeszekSwirski Can you sync your fork with our main branch. I had to do a forced update to fix problems I created earlier today.

Your PR seems quite reasonable and maybe we want to include it in a release very soon.

LeszekSwirski commented 3 months ago

Sure, done.

lemire commented 3 months ago

I will push a patch release immediately because I want your fix from an earlier PR. Meanwhile let us turn this PR into at least a minor release.

LeszekSwirski commented 3 months ago

Sounds reasonable, it's an observable API change if anyone checks pnr.lastmatch instead of pnr.valid. I could make it backwards compatible by adding a new const UC* error_location if you wanted? No strong opinion from my end.

lemire commented 3 months ago

@deadalnix Would you have a look? I think that this PR is a nice step up.

lemire commented 3 months ago

@LeszekSwirski I am not 100% sure that we have users of this part of our API. But it is more than a patch for sure.

deadalnix commented 3 months ago

The obvious thing that jump to my eyes here is the lack of tests :)

The feature looks good, the implementation fairly uncontroversial considering the conditions are already checked for, so for me, as long as there is a good test, I think it should go it.

LeszekSwirski commented 3 months ago

Fair comment :smile: I'll add some tests tomorrow.

lemire commented 3 months ago

@LeszekSwirski Thanks for adding tests.

lemire commented 3 months ago

This PR looks good to me. I am inviting further review.

This would be at least a minor release.

LeszekSwirski commented 3 months ago

Sorry, the formatting was a bad push, it wasn't intended for review

LeszekSwirski commented 3 months ago

@deadalnix ok now the formatting is fixed (just a "format on save" mess up)

lemire commented 3 months ago

ok now the formatting is fixed (just a "format on save" mess up

Thankfully, we have tools to automatically format. Although, for this project, we don't have a standard yet. If you are interested, I have a PR outstanding that would prescribe a given style:

https://github.com/fastfloat/fast_float/pull/257

lemire commented 3 months ago

The tests pass. We will merge this PR unless someone objects in the near future.

lemire commented 2 months ago

Merging. This will be part of the next release.