Skip to content
This repository has been archived by the owner on Aug 15, 2021. It is now read-only.

Add roundtrip example test case. #169

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

windelbouwman
Copy link

@windelbouwman windelbouwman commented Dec 31, 2019

Hi, I was wondering whether this testcase should work or not? What am I doing wrong in this case, or is this a bug?

---- test_roundtrip stdout ----
thread 'test_roundtrip' panicked at 'called `Result::unwrap()` on an `Err` value: ErrorImpl { code: Message("invalid type: floating point `2.5`, expected f64"), offset: 0 }', src/libcore/result.rs:1165:5

The error message seems to indicate that we got a floating point, but we wanted a f64 value, this must be possible right?

This example serializes a data structure, and then again de-serializes the structure.

@windelbouwman
Copy link
Author

Btw, I tried serde_json, and this crate works well with these lines:

let txt = serde_json::to_string(&stuff1).unwrap();
let stuff2 = serde_json::from_str(&txt).unwrap();

@pyfisch pyfisch added the bug label Jan 3, 2020
@pyfisch
Copy link
Owner

pyfisch commented Jan 3, 2020

This is an instance of #144

Basically don't use flatten until someone fixes this.

@windelbouwman
Copy link
Author

Any clue yet on how this should be fixed? I'm not into serde, so I do not yet grasp how it should work.

Also, the issue is more about the enum tag being improperly handled right? In this example, it's a field inside the flattened enum which behaves weird.

Should this be fixed in serde_cbor, or in serde itself?

@pyfisch
Copy link
Owner

pyfisch commented Jan 3, 2020

Any clue yet on how this should be fixed?

No, no idea. That it works with json is probably an indication that the problem lies within serde-cbor and not serde itself.

@windelbouwman
Copy link
Author

windelbouwman commented Jan 6, 2020

I found a solution to this issue. The solution was to always visit the f64 variant, since the cbor encoding is smart, and tries to use a compact value as possible, depending on the bits of the floating point number. This deserialization now starts to visit the f64 case (most bits), this will work onto f32 as well.

Please have a look at this, whether this makes sense or not.

@pyfisch
Copy link
Owner

pyfisch commented Jan 6, 2020

Code fails to compile because of warnings.

I am not sure that this change will work as some code actually needs a f32 and not a f64. I recall that there was a similar issue in the past but I can't find the id right now.

@windelbouwman
Copy link
Author

I added a field in the roundtrip test case with of type f32 to test this. Is there a sort of stress test for rountripping all serde types available?

@windelbouwman
Copy link
Author

Excuse me for the many commits, please squash merge if merging this work.

@windelbouwman
Copy link
Author

@pyfisch what do you think about this change? Is it good / bad?

I had some second thoughts about the packing / unpacking logic of floating point values. I noticed that if a f64 value can be represented as a f32 or even and f16 value, this will be done during encoding.

I think this probably is counter intuitive in one of the following ways:

  • Code on the decoding end (which may not be rust code) might expect the encoding to be an f64 type.
  • Packet size will vary, subsequent packets vary in size. Not necessarily bad, but might be unexpected.

@pickfire
Copy link

@windelbouwman Isn't using this patch use f64 rather than f16 or f32, which increases unused size? What if there are f128 (#103), do we need to switch all of them to f128 too?

@windelbouwman
Copy link
Author

@pickfire I do not understand what you mean. Is there a f128 type? Then probably we need to use it as well.

@pickfire
Copy link

That is in another pull request #103 like I mentioned.

@pyfisch
Copy link
Owner

pyfisch commented Feb 12, 2020

@pickfire #103 is only for integer types, not floats. So there is no f128.

@windelbouwman
Copy link
Author

@pyfisch what do you think of this change? Does it make sense?

@pyfisch
Copy link
Owner

pyfisch commented Mar 28, 2020

As discussed in #179 serde-cbor will be rewritten and I don't want to maintain it in the future. Therefore I am no longer reviewing patches for new features and minor bugs. While this change fixes the problem you encounter other users may depend on an f32 and their code would be broken (I think).

@windelbouwman
Copy link
Author

Okay, thanks for the heads up!

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

Successfully merging this pull request may close these issues.

None yet

3 participants