Skip to content
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> APIs #891

Closed
wants to merge 5 commits into from

Conversation

felipepessoto
Copy link
Contributor

Add List.CopyTo(Span destination) and List.CopyTo(int sourceIndex, int count, Span destination)

Fix https://github.com/dotnet/corefx/issues/33006


I watched the Design Review video but I still have some questions (this is my first non-trivial contribution).

  1. CopyTo(Span destination) should throw if Span.Length is smaller than List.Count?
  2. CopyTo(int sourceIndex, int count, Span destination) should throw if count is greater than destination Span?
  3. Do I need to use the ThrowHelper.ThrowXXX methods? Or can I just use throw new XException?
  4. Is expected to use Buffer.Memmove? I'm not familiar with this class.

Thanks

@safern @sywhang

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
Fix CopyTo(Span<T>) overload to pass the right size

Add test to ensure Copy Span doesn't copy more than List.Count when the backing array is larger than List
@felipepessoto
Copy link
Contributor Author

@jkotas is it my PR, or CI is broken?

@jkotas
Copy link
Member

jkotas commented Dec 17, 2019

CI is broken.

@felipepessoto felipepessoto changed the title [WIP] Add List<T>.CopyTo Span<T> APIs Add List<T>.CopyTo Span<T> APIs Dec 20, 2019
@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 891 in repo dotnet/runtime

1 similar comment
@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 891 in repo dotnet/runtime

@safern
Copy link
Member

safern commented Dec 23, 2019

/azp run runtime-coreclr

@safern
Copy link
Member

safern commented Dec 23, 2019

/azp run runtime-libraries

@dotnet dotnet deleted a comment from azure-pipelines bot Dec 23, 2019
@dotnet dotnet deleted a comment from azure-pipelines bot Dec 23, 2019
@carlossanlop
Copy link
Member

The new public APIs have been properly documented with triple slash comments. Thanks @felipepessoto. Removing the label.
Adding @safern as reviewer for System.Collections.

@safern
Copy link
Member

safern commented Feb 6, 2020

@carlossanlop, are we still adding this API?

I spoke with @eerhardt and @jkotas and I think we agreed that it makes sense to not add this APIs given we can achieve the same using CollectionsMarshal and also that in order to have a "complete" span support and also, adding these APIs might slow down other operations on List<T> so it seems reasonable to teach people to use CollectionsMarshal instead.

Sorry @felipepessoto for this and thanks for the effort on putting this together, looking forward to your next contribution 😄 -- will close this, if people disagree, we can of course re-open.

@safern safern closed this Feb 6, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants