-
-
Notifications
You must be signed in to change notification settings - Fork 705
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Partial fix of Issue 8384 - std.conv.to should allow conversion betwe… #4199
Conversation
|
| @@ -875,11 +875,19 @@ T toImpl(T, S)(S value) | |||
| ()@trusted{ memcpy(result.ptr, value.ptr, value.length); }(); | |||
| return cast(T) result; | |||
| } | |||
| else static if (isPointer!S && is(S : const(char)*)) | |||
| else static if (isPointer!S && (is(S : const(char)*) || is(S : const(wchar)*) || is(S : const(dchar)*))) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the rest of the constraint should use std.traits.isSomeChar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isSomeChar checks if it's a char of some sort, not a char* of some sort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isSomeChar!(ElementEncodingType!(S))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thankies.
| @@ -29929,7 +29929,7 @@ else version(Windows) | |||
| } | |||
|
|
|||
|
|
|||
| this(string name, TIME_ZONE_INFORMATION tzInfo) @safe immutable pure | |||
| this(string name, TIME_ZONE_INFORMATION tzInfo) @trusted immutable pure | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm more than a little unsure about this. The conversion on line 29934 is definitely wrong as is (conversion from wchar* to string does not work, and fails in ugly ways at runtime). The question is, should this function be @trusted? @System?
Should it even be using this conversion? Am I right to address the issue here or should that be a separate pull?
Regarding to the safety of using to!string(wchar*), MSDN claims in one place that DaylightName will always be zero-terminated. If this is true, this should be safe. However, there are many versions of the info page on TIME_ZONE_INFORMATION, and not all of them include this information. The constructor is private though, and I haven't found a case where it could be called with invalid data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that the consensus at this point is that if the @safety depends on the argument being correct, then it should be marked as @trusted. The reality of the matter in this case is that either will work. The constructor is private, and the function calling it is already @trusted. So, just mark it as @trusted.
And if the Windows API gives a wchar* that isn't zero-terminated, then it's just plain broken, since it's a C API. But actually, in this case, it doesn't even matter, because the TIME_ZONE_INFORMATION is constructed by std.datetime in the caller, and it's explicitly zero-terminated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks.
…en any pair of string/wstring/dstring/char*/wchar*/dchar*
|
LGTM |
|
Auto-merge toggled on |
…en any pair of string/wstring/dstring/char_/wchar_/dchar*
std.conv.to currently fails uglily with 'object.Error@(0): array cast misalignment.' when casting from a char* to any wider string type, and does not support any kind of conversion from e.g. a wchar* to wchar[]. This should fix both those issues.
Note that Issue 8384 also mentions conversions from {X}char[] to {Y}char*. I have not tackled that problem, as std.conv.to does not currently support any kind of conversion in that direction.