-
Notifications
You must be signed in to change notification settings - Fork 1k
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 collection literal support for Memory<T>
and ReadOnlyMemory<T>
.
#8144
Comments
Tagging subscribers to this area: @dotnet/area-system-memory |
If we want to enable this, it'd be better if either a) it was implemented in the compiler or b) a new builder pattern was supported by the compiler to enable it to create an array and do ownership transfer. Otherwise, the compiler is going to need to build up a span and then pass that off to these methods which will be forced to allocate a copy. For small spans of known size that can be done by the compiler on the stack, that's probably not a big deal. For larger spans, including those of unknown size due to the use of spreads, it's a bigger deal. |
I agree with Stephen. Best would be to open a proposal directly at csharplang. It's possible this might be small enough to squeeze in. But that would be an @jaredpar question. |
Thanks. Given there's consensus I'll transfer the issue to csharplang. |
@cston @RikkiGibson if we wanted to support arrays for the 'collection builder pattern' would this work be on the order of size as a bug fix? This is work I'd be willing to do. |
@eiriktsarpalis can you update the proposal to take arrays not spans. |
This is something I've been interested in, too. I've run into awkward situations where I wanted to spread into a ReadOnlyMemory method parameter and had to add an array cast. |
If I'm understanding this correctly there will be a future version of the compiler recognizing |
It shouldn't need new methods: {ReadOnly}Memory already have constructors accepting |
I think we'd need to review a proposal with language design to determine the scope of the change to collection builder support. |
This idea was considered by C# LDM at least once. My memory was that we decided to not go forward with this because
We're over booked for C# 13 at this point. Earliest would be C# 14. |
Since then, we've dropped that requirement: |
That's what i want this proposal to be :) |
While considering supporting |
Whether it is "bug fix level", I am not sure. It needs spec changes, LDM approval, and tests. I guess I would expect the PR for it to be a few thousand lines. It could probably be done right in main. It seems reasonable to add support for The only thing I wouldn't want to add is |
Note: teh original spec allowed for this with builders. Our LDM decisions where that we didn't like how broad this was getting in scope, and we wanted to keep the allowed patterns small here unless necessary. I like the idea of keeping the set of supported cases minimal. Specifically, keeping it just with arrays/spans seems ideal to me as they're the lowest level primitives for creating contiguous data (either on the heap or stack) and allowing the builder to take over from there. Also, it seems like it would keep costs much lower if we limit it just to arrays. |
works for me! |
We are over booked for C# 13 right now. I've already had to cut several items. Think this should be a candidate for 14 but do not want to add additional stress by putting this into 13. |
Would be nice if we could add collection literal support for Memory types:
CollectionBuilderAttribute-based proposal
API Proposal
Alternative Designs
Memory literal support could be implemented by the compiler directly, avoiding the need to copy elements from an intermediate span.
cc @stephentoub @CyrusNajmabadi
The text was updated successfully, but these errors were encountered: