Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

get_value<int>() and get_value<float>() reporting floating point underflow on value 0 #366

Closed
2 tasks
ARessegetesStery opened this issue Jun 20, 2024 · 13 comments
Labels
bug Something isn't working

Comments

@ARessegetesStery
Copy link

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 underflow

Expected 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

@fktn-k
Copy link
Owner

fktn-k commented Jun 20, 2024

@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.

@ARessegetesStery
Copy link
Author

@fktn-k Thanks for sharing the fix! I haven't looked into the source code and jumped to the conclusion lol.

@fktn-k
Copy link
Owner

fktn-k commented Jun 20, 2024

@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.

@ARessegetesStery
Copy link
Author

@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: 1

with a float in c++. I guess this will make the library easier to use.

@fktn-k
Copy link
Owner

fktn-k commented Jun 21, 2024

@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: 1

with 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++.

@fktn-k
Copy link
Owner

fktn-k commented Jun 30, 2024

@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.

@ARessegetesStery
Copy link
Author

@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 :)

@fktn-k
Copy link
Owner

fktn-k commented Jul 1, 2024

@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

@fktn-k fktn-k added the bug Something isn't working label Jul 2, 2024
@fktn-k
Copy link
Owner

fktn-k commented Aug 17, 2024

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?

@ARessegetesStery
Copy link
Author

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!

@fktn-k
Copy link
Owner

fktn-k commented Aug 18, 2024

@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?

@ARessegetesStery
Copy link
Author

@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!

@fktn-k
Copy link
Owner

fktn-k commented Aug 19, 2024

@ARessegetesStery
Thanks for the confirmation!
Feel free to reopen tjis issue if you notice anything unresolved in the above PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants