Add DateTime operators to SYSTEMTIME #672
Conversation
| if (sysTime.wYear <= 0 || sysTime.wMonth <= 0 || sysTime.wDay <= 0) | ||
| { | ||
| Debug.Fail("Incorrect SYSTEMTIME info!"); | ||
| return global::System.DateTime.MinValue; | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think this is acceptable to just return DateTime.MinValue in an error condition. For WinForms they're free to swallow errors if they want, but we can't do that in a general system like this.
An interesting scenario that this check includes (among others) is default(SYSTEMTIME), which evidently cannot be converted. Maybe we could do something special where we say if (sysTime == default) return default; in both conversion operators. Then at least structs with all zeros can convert between each other with no information loss and no exception. At that point, exceptions only get thrown when there is actually invalid data set into the struct.
Thoughts?
There was a problem hiding this comment.
As an aside, given this will be different behavior from what WinForms was used to, we should call out to them when they adopt this that it's different, and they should carefully review where they rely on their old implicit conversion operator to avoid regressions.
This BTW is an argument for not declaring implicit operators except where they can be trivially implemented. I'm still on the fence as to whether these should be implicit or explicit. When explicit, it would be far easier for WinForms to audit their code (because errors will be everywhere they used to use their implicit ones) and either allow use of an explicit operator or call into a method they define that retains their old proprietary behavior.
There was a problem hiding this comment.
I think I have implemented what you asked. Did we want to keep the default and the year/month/day comparison separate and throw an exception when those values are invalid?
I think they should be explicit. SYSTEMTIME for year "The valid values for this member are 1601 through 30827."
https://docs.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-systemtime
For DateTime "The year, between 1 and 9999."
https://docs.microsoft.com/en-us/dotnet/api/system.datetime.year?view=net-6.0
We will get data loss when converting between them.
There was a problem hiding this comment.
We will get data loss when converting between them.
If data loss is inevitable, they should not be implicit conversion operators.
If we work for all arguably valid data, and can throw in the unconvertible cases, that may qualify for implicit operators.
Otherwise, explicit operators would probably be better.
elachlan
force-pushed
the
SYSTEMTIME
branch
2 times, most recently
from
0423e89 to
1cef4b1
Compare
| sysTime.wMinute, sysTime.wSecond, sysTime.wMilliseconds); | ||
| } | ||
|
|
||
| public static explicit operator SYSTEMTIME(global::System.DateTime time) |
There was a problem hiding this comment.
This could maybe be implicit. But if it is ever used in a pinvoke it might cause issues because the minimum value for year is 1601.
https://docs.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-systemtime
There was a problem hiding this comment.
Let's leave it as explicit for now. It's safer to move from explicit to implicit later than the other way around.
|
@elachlan I'm on jury duty last week and next week, FYI. So my participation will be weak and delayed through the end of that. |
Add Tests for Template structs Fix up output formatting
| sysTime.wMinute, sysTime.wSecond, sysTime.wMilliseconds); | ||
| } | ||
|
|
||
| public static explicit operator SYSTEMTIME(global::System.DateTime time) |
There was a problem hiding this comment.
Let's leave it as explicit for now. It's safer to move from explicit to implicit later than the other way around.
|
Thanks, @elachlan |
Fixes #667 and #673