fix(json_render): json_render is not accurate enough for extremely sm…#6531
Conversation
…all numbers. eg: fmt::format_to(std::back_inserter(buffer), FMT_COMPILE({}), double(0.0000000000017114087924596788)); you will get a result of 1.7114087924, this is a completely wrong result.
|
@Rejudge-F This introduces a C++20 construct, but so far, this codebase is C++17. @mjjbell @SiarheiFedartsou How do you feel about bumping the minimum C++ version needed? |
I don't think we need to bump the minimum C++ version needed, because the format_to function used by osrm is supported by the third-party fmt-9.1.0(code's here: third_party/fmt-9.1.0 ). The above reference to the cpp reference is for the convenience of explaining the format_to parameters |
|
@danpat indeed, it seems @Rejudge-F is right and we can support it with C++17(on another hand std::format from C++20 would allow us to get rid from dependency). But in general I don't see a lot of reasons to not use C++20 at the moment - we don't have obligations to maintain compatibility with old compilers. |
Rejudge-F
force-pushed
the
bugfix-error_formatting_doubles
branch
from
7e0d760 to
16c5a64
Compare
unit_tests/util/json_render.cpp
Outdated
| BOOST_CHECK_EQUAL(str, "42"); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(test_json_issue_6531) { |
There was a problem hiding this comment.
@Rejudge-F Cool that you found this g suffix! Thanks! May I ask you to add 3-4 test cases more in order to make sure that the rest of the things work as expected? e.g. 42.1, 42.0, 0.12345678912345?
Let me know if you don't have time on it and I will try to do that on my own.
|
@SiarheiFedartsou About this issue test cases has added. |
|
Nice change. |
…F/osrm-backend into bugfix-error_formatting_doubles
|
Hey @Rejudge-F Will you be able to update it too please? |
Willing to |
|
I have fixed all cucumber issue. |
| When I route I should get | ||
| | from | to | route | turns | | ||
| | a | e | cap south,florida nw,florida nw,florida ne | depart,turn right,continue uturn,arrive | | ||
| | a | e | cap south,florida ne,florida ne | depart,turn left,arrive | |
There was a problem hiding this comment.
I am curious how it is possible that it was changed due to changes in JSON rendering? 🤔
There was a problem hiding this comment.
I guess it is due to some caches on CI because you have new failure which says you have "original" values here
https://pipelines.actions.githubusercontent.com/serviceHosts/5f8b3a21-777f-4ccd-83b2-9ebc854aa1e0/_apis/pipelines/1/runs/1148/signedlogcontent/31?urlExpires=2023-02-02T09%3A15%3A35.9673538Z&urlSigningMethod=HMACV1&urlSignature=k5UH1girtk3krsR2TdceIF1EsrnxhL2RtXruekV8s%2BU%3D
There was a problem hiding this comment.
This case is torturing me non-stop hahahaha, do you have a good solution?
There was a problem hiding this comment.
Yes, I think I can try dropping CI caches. Sorry, I am a bit busy right now, but will try to do that this evening(not sure which time zone you are, it will be in ~8-10 hours).
There was a problem hiding this comment.
Then this issue will be handed over to you, and I will no longer submit relevant code. I'm in GMT+8
|
@Rejudge-F many thanks for this contribution! |
|
Very nice contribution. Tyvm |
4 participants
fix(json_render): json_render is not accurate enough for extremely small numbers. eg: fmt::format_to(std::back_inserter(buffer), FMT_COMPILE("{}"), double(0.0000000000017114087924596788)); you will get a result of 1.7114087924, this is a completely wrong result.
formatter params(width & precision chapter): https://en.cppreference.com/w/cpp/utility/format/formatter
Issue
#6513
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?