Hi @lemire, nice to see you here. Do you think ffc is mature enough already? I see it has a lot of activity in the last days. I hope we can avoid updating it too often.

Please also add ffc.h so the code compiles. :)

Do you think ffc is mature enough already? I see it has a lot of activity in the last days.

It's a fair question; all of the activity since the first release has been around project management, establishing a versioning scheme, ci, tooling on the repo itself. We have made only one true code change since release, which was not a correctness fix.

The logic itself is of course very mature as it comes from fast_float and the port is direct. The question is around the accuracy of the port, but with all of the fast_float test suite passing, it seems unlikely there is any major miss.

I see the existing fast_float usage is already opt-in in valkey, that could be a good way to 'let it soak' for a little while before making it the default.

I see the existing fast_float usage is already opt-in in valkey, that could be a good way to 'let it soak' for a little while before making it the default.

Maybe it's not necessary. Now that we have a plain C solution, and we've had fast float for a year as opt-in, it seems safe to me to use ffc now without a fallback. Let's see what others think.

We made fast float opt-in just because it's C++ and we didn't want to change the toolchain requirements, but otherwise we want to avoid compile-time options. We want a uniform build and many users run on containers or other pre-built binaries and/or are not able to decide how it's compiled.

@dvkashapov

Yes, I think this can be vendored. I simply forgot to check in the file, which I just did now.

Please also add ffc.h so the code compiles. :)

Yeah. Sorry about that.

I see the existing fast_float usage is already opt-in in valkey, that could be a good way to 'let it soak' for a little while before making it the default. Do you think ffc is mature enough already?

I am quite happy that valkey depends on the library that I maintain (fast_float). It is indeed correct that fast_float is more mature. But yeah, I have confidence in ffc. It is up to the valkey folks to decide. (And, of course, to consider @kolemannix's opinion on the matter.)

Maybe some background is in order. Antirez raised a bit of attention when he announced that he had used AI to port fast_float to C so that redis would not need a C++ compiler at compile time. Seemed reasonable. Yet I personally do not think that the vibe coded 'C port' is good. Meanwhile, independently, @kolemannix did an actual port of fast_float. His port is not vibe coded. So there is discussion in the redis repo about adopting ffh, and so forth. It felt to me that valkey should have the same option.

If we can review this in the next day or so, we can merge this before 9.1 cutoff. This seems worth pulling in.

Codecov Report

❌ Patch coverage is 75.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.44%. Comparing base (fc217fc) to head (5ddb25f).
⚠️ Report is 15 commits behind head on unstable.

@@             Coverage Diff              @@
##           unstable    #3329      +/-   ##
============================================
- Coverage     75.05%   74.44%   -0.62%     
============================================
  Files           129      130       +1     
  Lines         71553    72553    +1000     
============================================
+ Hits          53705    54011     +306     
- Misses        17848    18542     +694     

util.c:(.text+0x29e1): undefined reference to `fast_float_strtod_n'

quick note - CI is failing because CMake is not updated to build/link the new fast_float_c_interface.

@sarthakaggarwal97 Let me have a look

Verified locally, I had not tested CMake. I will fix in a minute or so.

@lemire Would you like to remove the extra build step and wrapper interface? I think its worth it, as the aim is toolchain complexity reduction!

I could take a crack as well.

I did the simplification. It does not simplify a whole lot: you basically move the definitions from the deps directory to the src directory. It is probably more or less even in terms of lines of code.

BUT you do get right of an extra directory in deps, which is nice.

@zuiderkwast I trimmed some more fat.