Conversation
aardappel
left a comment
aardappel
left a comment
There was a problem hiding this comment.
Can you put some float formatting tests somewhere? With a wide variety of floats.. then run them both with the old and the new code.
| @@ -0,0 +1,4440 @@ | |||
| // Copyright 2020-2025 Junekey Jeon | |||
There was a problem hiding this comment.
This header contains a TON of general float functionality that is not needed for just the task of float 2 string.. I wonder if there is a more minimal version?
There was a problem hiding this comment.
I believe we can strip this down all we want -- I just brought in the files wholesale.
include/flatbuffers/util.h
Outdated
| ss << t; | ||
| auto s = ss.str(); | ||
| // Handle NaN | ||
| if (std::isnan(t)) { |
There was a problem hiding this comment.
Ok, so how does dragonbox output these? Does it handle them at all? I know its supposed to be fast, but is all this checking potentially slow things down?
There was a problem hiding this comment.
The is_decimal function states these are preconditions - if you pass them in you get an assertion failure.
There was a problem hiding this comment.
I'll note that the checks probably aren't anywhere near the slowest part of this (or any other part of the code that uses this) -- this code is riddled with std::string allocations :)
include/flatbuffers/util.h
Outdated
| } | ||
|
|
||
| // Handle zero (preserve sign of -0.0 if desired) | ||
| if (t == T{0}) { |
There was a problem hiding this comment.
Why does this need a special case?
There was a problem hiding this comment.
from the repo readme:
// Here, x should be a nonzero finite number!
// The return value v is a struct with three members:
// significand : decimal significand (1234 in this case);
// it is of type std::uint64_t for double, std::uint32_t for float
// exponent : decimal exponent (-3 in this case); it is of type int
// is_negative : as the name suggests; it is of type bool
auto v = jkj::dragonbox::to_decimal(x);
|
refactored to hide the dragonbox impl and add access to old functions. still need to add tests. |
|
@aardappel added some tests -- if you comment in the line I added to CMakelists.txt you can run the old method. |
|
alright, I got everything but windows happy - it is mad about some type casting that I will have to run down later. |
2 participants
Fixes #8573