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

Collection expressions: use inline array for span argument when the argument cannot escape #69348

Closed
wants to merge 2 commits into from

Conversation

cston
Copy link
Member

@cston cston commented Aug 2, 2023

Split out from #69291.

Relates to test plan #66418

@RikkiGibson RikkiGibson self-assigned this Aug 2, 2023
@Sergio0694
Copy link
Contributor

When using this on a runtime without [InlineArray], would this show an error or silently allocate an array? 🤔

@cston
Copy link
Member Author

cston commented Aug 4, 2023

When using this on a runtime without [InlineArray], would this show an error or silently allocate an array?

The proposal implemented here is to allocate an array on heap when the runtime does not support inline arrays.

@jcouv jcouv self-assigned this Aug 4, 2023
@cston cston marked this pull request as draft August 4, 2023 14:30
{
var elements = node.Elements;
return elements.Length > 0 &&
!elements.Any(i => i is BoundCollectionExpressionSpreadElement) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(unrelated) I'm going to sound like a broken record but didn't this ended up being an expression? Just asking because this pattern is already present in list-patterns except that this doesn't need explicit handling in the binder since the parser would never produce such node anywhere else. I'm willing to do the work to make it so if there's timing concerns. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you're asking here.

"but didn't this ended up being an expression?"

What are you referring to here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was asking about the syntax model for collection expressions and whether or not CollectionElementSyntax needs to exist to distinguish between spread and expression elements. Would it make sense to consider a SpreadExpression here? As mentioned above I don't think this involves any additional complexity in the parser.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing the value in a SpreadExpression. We'd also then need a KeyValueExpression. Neither of these could show up anywhere else, so i'm not sure why that would be a good thing. Conversely, having specific element types for a list matches what we do elsewhere, and provides clear informatino as to the strong types, and expected shapes, one would expect when processing these lists.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOte: further things might be a construct like if (x) y allowed as a collection-expression element. Making expressions out of all of these things (even though they would only be viable in one parent) is not a good thing IMO.

@cston
Copy link
Member Author

cston commented Aug 5, 2023

Replaced by #69412.

@cston cston closed this Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants