🐛 zv: Allow f32::NAN to be decoded too

[Spindel]

The range check would fail on NAN, INFINITY and NEG_INFINITY even if they are completely valid things to decode.

This does not promise that an encoded NAN will be bit-wise identical as the decoded NAN, but if signalling NAN is used that precisely the user can have a handful of more interesting problems.

Fixes #1145.

[zeenix]

Thanks! Is this WIP? I don't see any fixes in the PR.

[Spindel]

Thanks! Is this WIP? I don't see any fixes in the PR.

Should be a Fixes, Did I mess up and assign it only as an Issue: ref rather than Fixes ref? Meh. I'll rewrite the commit msg and force push if you don't have any other review comments.

[zeenix]

Should be a Fixes, Did I mess up and assign it only as an Issue: ref rather than Fixes ref? Meh. I'll rewrite the commit msg and force push if you don't have any other review comments.

I'm not referring to the wording. I don't see any code changes except addition of an assertion and a test case. Please check your changes. Perhaps you forgot to commit something?

[Spindel]

Should be a Fixes, Did I mess up and assign it only as an Issue: ref rather than Fixes ref? Meh. I'll rewrite the commit msg and force push if you don't have any other review comments.

I'm not referring to the wording. I don't see any code changes except addition of an assertion and a test case. Please check your changes. Perhaps you forgot to commit something?

That assert change is really all there is to it, as it treated NAN values as "too big" to transform, causing an panic when de-serializing an NAN value. Try it yourself with the assert change reverted and you'll see the test-case fails.

[zeenix]

That assert change is really all there is to it, as it treated NAN values as "too big" to transform, causing an panic when de-serializing an NAN value

Why not error out instead? I don't think assert is a solution.

[Spindel]

That assert change is really all there is to it, as it treated NAN values as "too big" to transform, causing an panic when de-serializing an NAN value

Why not error out instead? I don't think assert is a solution.

Cause the the code already used assert's and I didn't feel that a drive-by bug fix should change the behaviour that much.

[zeenix]

Cause the the code already used assert's and I didn't feel that a drive-by bug fix should change the behaviour that much.

It's not a behaviour change IMO. asserts are for invariants that must always hold. Clearly, that is not the case here.

[Spindel]

Wrapped up that change and folded the function into the deserializer.

usize_to_u8: Signature string converters seem to have some asserts on the length of them where they shouldn't be more than u8, but that seems mostly reasonable that the code will panic on when serializing.

The asserts in usize_to_u32 seems sane as noone should ever serialize more than 32bits worth of data at once.

[zeenix]

Now we're talking. :)

[zeenix]

Thank you for your contribution! 🙏 We hope you have read our contribution guideline and followed it to the best of your abilities: https://github.com/dbus2/zbus/blob/main/CONTRIBUTING.md#submitting-pull-requests

As per those guidelines, please add an emoji prefix to the commits/PR title. You can just c&p it from here.

[Spindel]

Emojis added, commits squashed. If there are more changes, you're welcome to do them.