adding the Cbor Deserializer#3877
Conversation
| double ReadDouble() { | ||
| auto type = m_decoder.PeekType(); | ||
| if (!type.has_value()) { | ||
| return 0.0; |
There was a problem hiding this comment.
let think about error handling here, every Read* has a
if (!type.has_value()) {
return default value
}branch in it. this one specifically sticks out to me because you would not be able to tell if the value ways truly 0.0 or a error. at the ShapeDeserializer interface level should we be returning a optoinal, maybe a outcome with a error that shows it was a deserialization error? we definitely need some sort of signal to the called that a error occurred
There was a problem hiding this comment.
Okay, I'll either add per-field Optional or an error flag like CRT's decoder->error_code:
d.BeginStruct();
d.EndStruct();
if (d.HasError()) return MakeError(d.GetError());
| if (type.has_value() && *type == CborType::IndefMapStart) { | ||
| m_decoder.ConsumeNextSingleElement(); | ||
| } else { | ||
| m_decoder.PopNextMapStart(); |
There was a problem hiding this comment.
How come we're discarding the map size for definite length struct? Server can send definite length here, and then our EndStruct that's relying on theIsBreak loop wouldn't work right? Lmk if im missing something
There was a problem hiding this comment.
You're right — the CRT doesn't synthesize a break for definite-length maps (it uses count-based iteration internally but doesn't expose it to PeekType callers). I'll change BeginStruct to return size_t, same as BeginList/BeginMap already do.
| DateTime ReadTimestamp() { | ||
| auto tag = m_decoder.PopNextTagVal(); | ||
| (void)tag; | ||
| return DateTime(static_cast<double>(ReadLong())); |
There was a problem hiding this comment.
Timestamps can also be floats. Also, ReadLong() can return a negative value. Negative timestamps per the RFC:
Negative values (major type 1 and negative floating-point numbers) are interpreted as determined by the application requirements as there is no universal standard for UTC count-of-seconds time before 1970-01-01T00:00Z
What we currently do ensures that if we encounter a negative value we error out
There was a problem hiding this comment.
I'll handle the float case and add validation for negative values.
3 participants
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.