-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Conversation
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.
Thanks!
@@ -409,6 +409,16 @@ public static string Join(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, Re | |||
return JoinInternal(path1, path2, path3); | |||
} | |||
|
|||
public static string Join(string path1, string path2) |
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.
These are going to end up treating null strings the same as empty strings. Do we instead want to match the behavior of Path.Combine, which throws ArgumentNullException for null strings?
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 key thing here is that we've shipped this way already. As is illustrated here, ((string)null).AsSpan()
gives you default, it doesn't throw. Same goes for the implicit conversion- presumably adding the throw would be a breaking change for those relying on that.
Outside of that I'd rather they don't throw as they'll never throw for the span versions. I'm more inclined to be consistent within the overloads. I don't plan to ever add a span version to Combine as I want to discourage usage- if I thought that likely I might sway a little, but still not enough to want to throw I think.
@pjanotti do you have an opinion?
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 key thing here is that we've shipped this way already. As is illustrated here, ((string)null).AsSpan() gives you default, it doesn't throw.
Ugh, you're right. Ok, we should keep it the way it is; otherwise adding these overloads would change behavior against 2.1 when code is recompiled.
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 can do Path.Join(String.Empty, (string)null)
today. It compiles and runs fine thanks to implicit string->span conversion. This code will pick up the new string overload once it is added. It is preferable for the new string overload to have same behavior as the Span version to avoid breaking change on recompile.
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.
Right, that's what I was agreeing with.
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
Commit migrated from dotnet/coreclr@5b23086
coreclr part of https://github.com/dotnet/corefx/issues/30049
corefx PR are ready to go dotnet/corefx@master...MarcoRossignoli:joinoverloads
/cc @JeremyKuhne