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 params ReadOnlySpan<T> overloads #100898

Merged
merged 18 commits into from
Apr 15, 2024
Merged

Add params ReadOnlySpan<T> overloads #100898

merged 18 commits into from
Apr 15, 2024

Conversation

jozkee
Copy link
Member

@jozkee jozkee commented Apr 11, 2024

Fixes #77873

@dotnet-issue-labeler dotnet-issue-labeler bot added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Apr 11, 2024
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@jozkee jozkee added area-System.Memory and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 11, 2024
@jozkee
Copy link
Member Author

jozkee commented Apr 11, 2024

I'm really scratching my head trying to understand why some projects fail to build. That's also why I sent this as draft.
cc @dotnet/area-infrastructure-libraries

Unix:
CSC : error CS0656: Missing compiler required member 'System.Runtime.InteropServices.MemoryMarshal.CreateReadOnlySpan' [/__w/1/s/src/libraries/System.IO.Pipes.AccessControl/src/System.IO.Pipes.AccessControl.csproj::TargetFramework=net9.0]
CSC : error CS0656: Missing compiler required member 'System.Runtime.InteropServices.MemoryMarshal.CreateReadOnlySpan' [/__w/1/s/src/libraries/System.Threading.Tasks.Dataflow/src/System.Threading.Tasks.Dataflow.csproj::TargetFramework=net9.0]
CSC : error CS0656: Missing compiler required member 'System.Runtime.InteropServices.MemoryMarshal.CreateReadOnlySpan' [/__w/1/s/src/libraries/System.Threading.Channels/src/System.Threading.Channels.csproj::TargetFramework=net9.0]
CSC : error CS0656: Missing compiler required member 'System.Runtime.InteropServices.MemoryMarshal.CreateReadOnlySpan' [/__w/1/s/src/libraries/System.IO.FileSystem.AccessControl/src/System.IO.FileSystem.AccessControl.csproj::TargetFramework=net9.0]
CSC : error CS0656: Missing compiler required member 'System.Runtime.InteropServices.MemoryMarshal.CreateReadOnlySpan' [/__w/1/s/src/libraries/System.Collections.NonGeneric/src/System.Collections.NonGeneric.csproj]
CSC : error CS0656: Missing compiler required member 'System.Runtime.InteropServices.MemoryMarshal.CreateReadOnlySpan' [/__w/1/s/src/libraries/System.Net.Http.Json/src/System.Net.Http.Json.csproj::TargetFramework=net9.0]

Windows:
CSC : error CS0656: Missing compiler required member 'System.Runtime.InteropServices.MemoryMarshal.CreateReadOnlySpan' [D:\a\_work\1\s\src\libraries\System.IO.IsolatedStorage\src\System.IO.IsolatedStorage.csproj::TargetFramework=net9.0-windows]

@stephentoub
Copy link
Member

@jozkee, try adding <Reference Include="System.Memory" /> to the .csproj.

That said, I'm not sure why CreateReadOnlySpan would be a required member, since I'd have expected there to be fallback strategies if it's not available (that said, I'm glad this error'd for us, as otherwise presumably we would have been silently falling back to slower support). @AlekseyTs?

@jozkee
Copy link
Member Author

jozkee commented Apr 11, 2024

FWIW: I brute-forced it and noticed the error goes away if I exclude the new string.Join overloads. @stephentoub Adding a reference to System.Memory fixes it too, is that the path we want to follow?

@stephentoub
Copy link
Member

Adding a reference to System.Memory fixes it too, is that the path we want to follow?

Yes

@jozkee jozkee marked this pull request as ready for review April 11, 2024 21:40
@AlekseyTs
Copy link
Contributor

@jozkee, try adding <Reference Include="System.Memory" /> to the .csproj.

That said, I'm not sure why CreateReadOnlySpan would be a required member, since I'd have expected there to be fallback strategies if it's not available (that said, I'm glad this error'd for us, as otherwise presumably we would have been silently falling back to slower support). @AlekseyTs?

I assume a ReadOnlySpan overload is getting chosen. What is needed to create one is actually defined by "Collection Expressions" feature. CC @cston.

@jozkee jozkee merged commit 64a937c into dotnet:main Apr 15, 2024
8 checks passed
@jozkee jozkee deleted the paramsspan branch April 15, 2024 20:22
jozkee added a commit that referenced this pull request Apr 16, 2024
@jozkee jozkee added this to the 9.0.0 milestone Apr 16, 2024
akoeplinger pushed a commit that referenced this pull request Apr 17, 2024
jozkee added a commit to jozkee/runtime that referenced this pull request Apr 19, 2024
jozkee added a commit to jozkee/runtime that referenced this pull request Apr 19, 2024
jozkee added a commit that referenced this pull request Apr 23, 2024
* Reapply "Add params ReadOnlySpan<T> overloads (#100898)" (#101123)

This reverts commit 3e569f5.

* Comment-out params keyword

* Remove /*params*/ from ref
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
* Add params span overloads

* Complete src/ref changes

* Add tests for MemoryExtensions, String, and StringBuilder

* Update tests for ImmutableHashSet, ImmutableList, ImmutableQueue, ImmutableSortedSet, and ImmutableStack

* Update tests for System.Diagnostics.Metrics

* Update tests for Console.Write[Line]

* Update tests for Deleate.Combine

* Update tests for IndentedTextWriter.Write[Line]

* Update tests for Path.Combine and Path.Join

* Update tests for StreamWriter and TextWriter

* Update tests for JsonArray and JsonTypeInfoResolver.Combine

* Update tests for System.Threading

* Add project reference to System.Memory where needed

* Fix missing params in ImmutableSortedSet.cs

* Fix error CS9220 in System.Dynamic.Runtime.Tests

* Suppress CS9220 in System.Dynamic.Runtime.Tests

* Address review feedback

---------

Co-authored-by: Stephen Toub <[email protected]>
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
…101308)

* Reapply "Add params ReadOnlySpan<T> overloads (dotnet#100898)" (dotnet#101123)

This reverts commit 3e569f5.

* Comment-out params keyword

* Remove /*params*/ from ref
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
…101308)

* Reapply "Add params ReadOnlySpan<T> overloads (dotnet#100898)" (dotnet#101123)

This reverts commit 3e569f5.

* Comment-out params keyword

* Remove /*params*/ from ref
@github-actions github-actions bot locked and limited conversation to collaborators May 17, 2024
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.

[API Proposal]: params ReadOnlySpan<T> overloads for existing params T[] overloads
3 participants