cbor icon indicating copy to clipboard operation
cbor copied to clipboard

Add roundtrip example test case.

Open windelbouwman opened this issue 4 years ago • 16 comments

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 avatar Dec 31 '19 15:12 windelbouwman

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();

windelbouwman avatar Dec 31 '19 15:12 windelbouwman

This is an instance of #144

Basically don't use flatten until someone fixes this.

pyfisch avatar Jan 03 '20 20:01 pyfisch

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?

windelbouwman avatar Jan 03 '20 20:01 windelbouwman

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.

pyfisch avatar Jan 03 '20 20:01 pyfisch

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.

windelbouwman avatar Jan 06 '20 20:01 windelbouwman

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.

pyfisch avatar Jan 06 '20 20:01 pyfisch

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 avatar Jan 06 '20 20:01 windelbouwman

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

windelbouwman avatar Jan 06 '20 21:01 windelbouwman

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

windelbouwman avatar Jan 29 '20 19:01 windelbouwman

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

pickfire avatar Feb 11 '20 03:02 pickfire

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

windelbouwman avatar Feb 11 '20 19:02 windelbouwman

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

pickfire avatar Feb 12 '20 05:02 pickfire

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

pyfisch avatar Feb 12 '20 09:02 pyfisch

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

windelbouwman avatar Mar 27 '20 20:03 windelbouwman

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

pyfisch avatar Mar 28 '20 09:03 pyfisch

Okay, thanks for the heads up!

windelbouwman avatar Mar 28 '20 10:03 windelbouwman