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

Unexpected behavior when deserializing to double #434

Closed
f-p-b opened this issue May 26, 2024 · 3 comments · Fixed by #436
Closed

Unexpected behavior when deserializing to double #434

f-p-b opened this issue May 26, 2024 · 3 comments · Fixed by #436

Comments

@f-p-b
Copy link

f-p-b commented May 26, 2024

Deserializing to double using:

ryml::NodeRef node = tree["key"];
double value;
node >> value;

The following two cases behave unexpectedly:

  • When the key is followed by no value it gives a segmentation fault
  • When the key is a number with a letter in a position that is not the first it returns 0 instead of giving an error.
@biojppm
Copy link
Owner

biojppm commented May 27, 2024

* When the key is followed by no value it gives a segmentation fault

TL;DR you should never call >> if you are unsure the value is empty; use .val_is_null() to check for this before calling, or use .get_if().

You cannot deserialize a val which does not exist, ie you cannot deserialize a null val. This is stated really early in the quickstart (and see also a sample for null values).

As for the error, it is actually an assertion, and as stated in the README the default error handler calls abort(). Are you sure it is a segfault, or is it an abort?

Also, I guess you're using an older version; in the latest release there are some important error handling fixes; see the release notes for more info.

* When the key is a number with a letter in a position that is not the first it returns 0 instead of giving an error.

Please provide concrete examples; the alphabetic characters matter, and their position as well.

Also, there was a fix in 0.6.0 that may be relevant for this problem; see #390 and #415.

@f-p-b
Copy link
Author

f-p-b commented Jun 2, 2024

Thank you for the explanations and my bad I missed the part about using >> on empty values in the quick start.

Are you sure it is a segfault, or is it an abort?

After further testing I determined its a segmentation fault only when I deserialize to a double or float. Other data types tested lead to an abort.

Regarding the second issue a small correction, it does not return zero but only the numbers before the letter(s). For example:
key: 0.r3 will return 0
key: 34gf3 will return 34
This is the case when I deserialize to a double or float. When deserializing to an integer it leads to an expected "could not deserialize value" error.

Note: I am using the master branch of about a week ago (commit e432c2f)

@biojppm
Copy link
Owner

biojppm commented Jun 6, 2024

I've investigated this.

  • for the empty value, there was a missing check for empty string in the read(ConstNodeRef, T*) functions. This was causing the segfault. I've added that check, such that an error is triggered.
  • the difference in behavior for key: 34gf3 between integers and floats is indeed there. Unfortunately, this happens because the fast_float parser is doing that. This is not solveable. If you need to handle values in your data which may not be a number, you can use csubstr::first_real_span() to ensure that all is good before deserializing. Something like this:
    csubstr val = node.val();
    if(val.first_real_span() == val)
       node >> var;
    else
       ERROR("not a real");

For an integer, the corresponding function would be either .first_int_span() or .first_uint_span(), as appropriate. These functions will return an empty string when the value is not fully a number (eg 34gf3 but also 34 gf3).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants