-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add List<T>.CopyTo(Span<T>) overloads #27721
Comments
Maybe we should make a ticket to generally overhaul all collection classes with Upgrading everything is necessary anyway, reduces the total amount of work, increases version predictability and helps with API consistency. |
@eerhardt would it make sense to wait for |
This would be really useful, when you have spans not backed by an array. It also seems a very easy implementation, and would match the span overloads being added to so many other APIs. I'd be happy to write it if it gets approved ( for what it's worth, I don't think the |
Note to implementer: be mindful that you might not be able to implement the |
Wow, that took quite a bit 😄
public partial class List<T>
{
// public void CopyTo(int index, T[] array, int arrayIndex, int count);
// public void CopyTo(T[] array, int arrayIndex);
// public void CopyTo(T[] array);
public void CopyTo(Span<T> destination);
public void CopyTo(int sourceIndex, int count, Span<T> destination);
} |
@terrajobst is anyone working on that? I would like to implement it |
@felipepessoto nobody yet. Please feel free to put up a PR in https://github.com/dotnet/runtime 😄 Assigning this issue to you. |
Add List<T>.CopyTo(Span<T> destination) and List<T>.CopyTo(int sourceIndex, int count, Span<T> destination) Fix https://github.com/dotnet/corefx/issues/33006
We have introduced
Are these specialized APIs worth it to save the few characters and allow writing just |
@jkotas Can you point me to where I can find |
@GSPP https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/CollectionsMarshal.cs I was't finding it too,if you search for CollectionMarshal (singular), GitHub doesn't return any results :) |
I think we totally missed the addition of the extension methods to For the second proposed API where we accept and index and length, ppl would have to slice the span, right? Also, if we go that route, would it make sense to add extension methods for collection interfaces? |
The CollectionsMarshal method is not an extension methods (and is under InteropServices namespace), I think that is part of the reason people doesn't find it, it is not very discoverable. |
We briefly discussed the CollectionMarshal.AsSpan in the API Review meeting, but it quickly got dismissed since the method isn't discoverable (which is by design of CollectionMarshal.AsSpan). If we want to start promoting
The place where this API proposal originated was in ML.NET when switching from using Arrays in VBuffer to using Spans. See |
Do you expect this to be a common operation? The ML.NET examples seems to be modernization glue: Part of the codebase was converted to use Spans, but other part is still on the classic plan. I think CollectionsMarshal is appropriate to use to marshal between two words efficiently. You are right that CopyTo can help in some situations like this, but other situations are still going to require CollectionsMarshal. Note that these APIs are not 100% pay-for-play. They will make all existing instantiations of |
I'm not sure I follow this statement. The scenario where this is used is where an unknown number of things need to be put in a Span. You pass around a
You mean because of the additional code on |
Is there any intention of creating collection interfaces with spans? It seems there could be some more comprehensive overhaul. That's difficult for sure without creating a mess. But maybe that's better than adding a span overload here and there, and dribble it out over many releases. |
Yes, the extra methods on frequently used generic types are not 100% pay-for-play. In particular, they tend to hurt with full AOT binary sizes. These two methods are small enough that it is not end of the world. We have done similar paper-cuts before. It would be useful to know what "done" looks like as @GSPP pointed out. |
E.g. #1517 |
Given the above discussion and #891 (comment) I'm going to close this issue. If people disagree or have any other thoughts, feel free to add them and we can always re-consider. |
We have
List<T>
CopyTo methods that all take an array:We should add similar overloads that take in a
Span<T>
.I know you can work around not having this API by looping over the
List<T>
yourself. But since List is backed by an array, it is faster to usememcpy
(or similar) than it is doing one element at a time. (That is the same reason why we haveCopyTo(T[])
in the first place.)The text was updated successfully, but these errors were encountered: