`get_value<int>()` and `get_value<float>()` reporting floating point underflow on value 0
Closed this issue · 13 comments
Description
When querying nodes with value 0, get_value<int>() and get_value<float>() will throw exception with message
Floating point value underflow detected.
It is suspected that a prior check on whether the value is zero should be conducted
Reproduction steps
Use yaml file (e.g. test.yaml)
pos: [0.0, 0.0, 0.0]and c++ code
std::ifstream ifs("test.yaml");
fkyaml::node root = fkyaml::node::deserialize(ifs);
auto posNode = root["pos"];
float posX = posNode[0].get_value<float>(); // throws exception reporting underflowExpected vs. actual results
Expected: reading in 0 correctly.
Actual: throwing exception "Floating point value underflow detected."
Minimal code example
No response
Error messages
No response
Compiler and operating system
g++; Windows
Library version
v0.3.8
Validation
- The bug also occurs if the latest version from the
developbranch is used. - I can successfully compile and run the unit tests.
@ARessegetesStery
Thanks for filing a issue.
I'll share what I've found while reproducing the issue.
Regarding the underflow check for floating point values, I've found a bug where the implementation mistakenly call std::min(), not std::lowest(), to get the minimum possible value of a target type.
So getting floating point values equal to/less than 0 will emit the same error.
@fktn-k Thanks for sharing the fix! I haven't looked into the source code and jumped to the conclusion lol.
@ARessegetesStery
Oh I stole the fun part, sorry. LOL
Actually, I couldn't have time to even reproduce the ingeter version of the bug.
I see you forked this repository, are you trying to fix it?
If so, that's very welcome.
@fktn-k Oh thanks for explaining - Glad to see your insights on the problem :)
Yeah I am planning to spend some time this weekend test this thoroughly and see if I can figure out a fix.
Also I am trying to see if this can be made more compatible, for example whether it is possible to read
someint: 1with a float in c++. I guess this will make the library easier to use.
@ARessegetesStery
OK. I'm looking forward to the fruits.
Feel free to ask anything.
There are a lot of templates along the way to the conversions, so it may be hard to understand.
whether it is possible to read
someint: 1with a float in c++. I guess this will make the library easier to use.
I like it. Reasonable and intuitive.
It's also close to the way primitives are handled in C++.
@ARessegetesStery
How does the investigation goes?
Maybe it's too complicated to handle in a single PR.
If so, we can separate the issue into a bugfix and improvements.
Like first just fixing the bug in conversions, and then adding improvements like you mentioned above.
@fktn-k
Sorry for the slow progresses - my computer somewhat broke last week and the new one is right on the way. Things should become much smoother after that.
Yep indeed I have opened issues for all features, separately, on my forked repo. If you want, I can also copy them here. For each fix I will open a separate pull request for clarity.
Also since github now has auto-sync functionalities for forked branches there's no need to wait for my updates to push your other commits to the main branch. I can sync the progress (and merge them if necessary) within a couple of clicks :)
@ARessegetesStery
Thanks for the updates and sorry for your computer...
And no worries. I just wanted to help if you were being stucked somewhere due to this library.
So anyway, take your time!
there's no need to wait for my updates
Thanks for your concern, but honestly I just couldn't spare enough time for this project lol
Hi, @ARessegetesStery.
I'm thinking about making a release this weekend because it's been too long since the last one with some other fixes staying in the develop branch.
Before the release, however, I just want to make the node-to-float conversion bug resolved.
So, can I take the bugfix part if you're not working on it?
Hi @fktn-k
Sorry it has been a really busy summer for me and I haven't got enough time to look into the code and fix this.
Please do the fix - thank you very much!
@ARessegetesStery
It happens quite often. Don't worry about it!
I just fixed the error in the PR #371.
You can now get whatever valid floating point values from nodes in the develop branch.
Can you confirm the fix if you have time?
@fktn-k
I was asleep last time and wasn't able to reply in time lol
Indeed that fixes the problem! And congrats on the new release!
@ARessegetesStery
Thanks for the confirmation!
Feel free to reopen tjis issue if you notice anything unresolved in the above PR.