Use Unsafe.As for generic cast
#118684
Use Unsafe.As for generic cast
#118684
Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-collections |
| @@ -161,7 +161,7 @@ private void AddRange(IEnumerable<KeyValuePair<TKey, TValue>> enumerable) | |||
| } | |||
| else if (enumerable.GetType() == typeof(List<KeyValuePair<TKey, TValue>>)) | |||
| { | |||
| span = CollectionsMarshal.AsSpan((List<KeyValuePair<TKey, TValue>>)enumerable); | |||
| span = CollectionsMarshal.AsSpan(Unsafe.As<List<KeyValuePair<TKey, TValue>>>(enumerable)); | |||
There was a problem hiding this comment.
Thanks, but I do not want to introduce unsafe code for things like this. If it's material, the cast should be elided by the JIT, if it's not already.
There was a problem hiding this comment.
@EgorBo The diffs show the cast is not elided.
There was a problem hiding this comment.
Thanks, but I do not want to introduce unsafe code for things like this.
We already use the unsafe method CollectionsMarshal.AsSpan here.
There was a problem hiding this comment.
Thanks, but I do not want to introduce unsafe code for things like this.
We already use the unsafe method
CollectionsMarshal.AsSpanhere.
That doesn't mean we need to add more. They're also "unsafe" in very different ways. Unsafe.As lets you completely break type safety. AsSpan is on the marshal class only because there's a potential for someone to make changes that get lost if they get the span, then edit the list in a way that causes it to grow, and then edit the span; it's not a memory safety issue.
There was a problem hiding this comment.
@EgorBo The diffs show the cast is not elided.
Yes, this is a known issue, I can't find the exact github issue, but I do remember it exists. Basically, the same repro I mentioned here: #118655 (comment)
I'll try to fix it in .NET 11.0. In case if you're looking for an inspiration on what to contribute to dotnet/runtime repo, anything that replaces unsafe code with safe (ideally, with zero overhead) would be more than appreciated 🙂
There was a problem hiding this comment.
For reference, this is the issue:
stephentoub
added
the
NO-MERGE
Improvements diffs expected.
Related: #118655