Fix not throwing due to overflow while writing NodaTime's period #5894
Conversation
|
|
||
| writer.WriteInt64(microsecondsInDay); | ||
| writer.WriteInt32(value.Weeks * 7 + value.Days); // days | ||
| writer.WriteInt32(value.Years * 12 + value.Months); // months | ||
| writer.WriteInt32(days); |
There was a problem hiding this comment.
Any reason to not just have the Write operations within the block above (at which point you don't need to separate the variable declarations)?
There was a problem hiding this comment.
Wouldn't that make anything what Write operation calls to also execute as unchecked? I do not think it should break anything now, but given how easy it will be to break something due to completely unrelated change, it's probably better to be as precise as possible.
There was a problem hiding this comment.
I don't think check { } affects called code - in my mental model it only affects code directly (lexically) within the block...
There was a problem hiding this comment.
Looked at the docs and yeah, you're absolutely right. That's good to know. Though with how we now have a try/catch there we might as well just leave it as is.
There was a problem hiding this comment.
It seems like simpler code to just move it all inside the checked and try, but I don't really care - up to you.
| { | ||
| Years = int.MaxValue | ||
| }; | ||
| await AssertTypeUnsupportedWrite<Period, OverflowException>(periodBuilder.Build(), "interval"); |
There was a problem hiding this comment.
Ideally, rather than a cryptic OverflowException we'd throw an ArgumentException (I think) with a clear message, saying that the provided Period is out of range for the PG interval type.
There was a problem hiding this comment.
Hmm, I suppose we can just put checked in try/catch and handle it there.
There was a problem hiding this comment.
Related to this I hope we'll get non throwing overflow checking operations at some point dotnet/runtime#13026
Fixes #5893