biojppm / rapidyaml

Rapid YAML - a library to parse and emit YAML, and do it fast.
MIT License
583 stars 100 forks source link

Unexpected behavior when deserializing to double #434

Closed f-p-b closed 5 months ago

f-p-b commented 5 months ago

Deserializing to double using:

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

The following two cases behave unexpectedly:

biojppm commented 5 months ago
* 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 commented 5 months ago

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 commented 5 months ago

I've investigated this.