-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[API Proposal]: params ReadOnlySpan<T> overloads for existing params T[] overloads #77873
Comments
Tagging subscribers to this area: @dotnet/area-system-memory Issue DetailsBackground and motivationC# 12 is tentatively on a path to add support for (It's likely that params in general will be de-emphasized once collection literals also comes to the scene, hopefully also in C# 12. As such, the benefits of adding params here would primarily be about making existing code more efficient. I've not included in this write-up other places we might want additional span-based overloads where there aren't already API ProposalThis is a rough strawman, to be refined as the language features are refined.
Additionally, the following already have
API UsageExisting usage. Alternative DesignsNo response RisksNo response
|
params IEnumerable proposal is more generic, it doesn’t require ReadOnlySpan. |
@NN--- |
Right, there's no benefit to us adding a |
I just re-went through all of our APIs in dotnet/runtime that either have params arrays today or have ReadOnlySpan parameters today, and prototyped out adding/updating everything that seems relevant. I've updated the top post with a revised concrete proposal to review, as the params span work should be landing in the compiler shortly. I've left out APIs that I would have liked but that at present don't appear like they'd yield any benefits (e.g. params span overloads for all the various Log methods on LoggerExtensions would be nice, but at present we'd need to immediately ToArray it for implementation reasons, defeating the benefits). |
It would be good to also add File span APIS: #99823 |
Can you share an example of where you'd want to use those as params? You're thinking about someone doing: byte a = ..., b = ..., c = ...;
File.WriteAllBytes(path, a, b, c); ? |
"I don't always write my files byte by byte, but when I do, I prefer params" 🤣 |
@stephentoub few things:
i'd be happy to help with the top section for now, and then come back and discuss doing parts of the bottom section. |
Thanks for offering to help. It's basically done, just some tests to be added. We were waiting to put up a PR for it until the compiler support landed in this repo, which it just did a few days ago. |
great, thanks! looking forward to seeing this in net9. :) |
EDITED 1/31/2024 by @stephentoub
Adding params to existing spans:
Adding new params span-based overloads (all of these have an existing corresponding
params T[]
):Background and motivation
C# 12 is tentatively on a path to add support for
params ReadOnlySpan<T>
, and for that to take precedence overparams T[]
. That means in APIs where we currently have aparams T[]
-based overload, there’s a benefit to us adding aparams ReadOnlySpan<T>
overload: it’ll be preferred by the compiler, and the compiler will aim to stackalloc the span rather than using an array. As a result, any existing call sites for the params-based overload will, upon recompilation, switch to the span-based overload and allocate less.(It's likely that params in general will be de-emphasized once collection literals also comes to the scene, hopefully also in C# 12. As such, the benefits of adding params here would primarily be about making existing code more efficient. I've not included in this write-up other places we might want additional span-based overloads where there aren't already
params T[]
-based overloads; we can continue adding such APIs via separate, dedicated proposals.)API Proposal
This is a rough strawman, to be refined as the language features are refined.
Additionally, the following already have
ReadOnlySpan<T>
-based overloads where there's also aparams T[]
-based overload, so we'd just need to addparams
to the existing method:API Usage
Existing usage.
Alternative Designs
No response
Risks
No response
The text was updated successfully, but these errors were encountered: