feat: Integration with fast_float #1170
Conversation
…or replacing strtod
|
@parthpatel FYI. If possible we would rather not take the C++ dependency, although that might be doable. Do you know how much effort it would be to port the code into C. The templating code seems very generic, but I don't think we need all the flexibility it is providing. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #1170 +/- ##
============================================
+ Coverage 70.69% 70.72% +0.02%
============================================
Files 114 114
Lines 61693 63006 +1313
============================================
+ Hits 43611 44558 +947
- Misses 18082 18448 +366 |
|
@madolson do you suggest to port the cpp code to c rather than use fast_float? |
Even if we remove the templating for character type, we still have "class" and "std::" library references. With little more effort, we should be able to remove it. But we won't be able to merge fixes or changes from upstream. In my opinion, we should start with fast_float library as a sub-module (instead of copying the code) and if we run into C++ related issues, then we can still internally change the implementation. |
| @@ -0,0 +1,3869 @@ | |||
| // fast_float by Daniel Lemire | |||
There was a problem hiding this comment.
if you embed this file, and in future there are bugfixes to this library, how do we track it? is it possible to use submodules for this https://git-scm.com/book/en/v2/Git-Tools-Submodules ?
There was a problem hiding this comment.
The proper scenario is to update the header file with the bug fix version. The diff will be large, but we will be kind of updating the third party dep. However, if we plan to make bug fix, it's not a good idea. Best would be to create a fork and make sure its parallel to the upstream, and we can make custom fixes as well. But it then makes additional overhead.
| double result; | ||
| auto answer = fast_float::from_chars(nptr, nptr + strlen(nptr), result); | ||
|
|
||
| if (answer.ec == std::errc()) |
There was a problem hiding this comment.
what is this check doing? shouldn't you just check for null?
There was a problem hiding this comment.
fast_float is written in a way where it returns error and we have to handle them explicitly. That's present in their docs.
|
|
||
| if (answer.ec == std::errc()) | ||
| { | ||
| if (endptr) |
There was a problem hiding this comment.
you can move this outside of if block, it is common.
|
|
||
| extern "C" | ||
| { | ||
| double fast_float_strtod(const char *nptr, char **endptr) |
There was a problem hiding this comment.
@madolson do we always use double type in Redis? Would we never want to use float?
|
@swaingotnochill I couldn't push changes to your PR, so I created what I was proposing as a draft PR here #1260 |
See #1069
This is a draft pull request and the work is in progress.
The goal of this is to get timely feedback and reviews from the reviewers.
TODO:
cc: @madolson @PingXie