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 ArgumentKind.ParamCollection #72221

Merged

Conversation

AlekseyTs
Copy link
Contributor

@AlekseyTs AlekseyTs commented Feb 22, 2024

Related #72222.

@AlekseyTs AlekseyTs marked this pull request as ready for review February 22, 2024 01:48
@AlekseyTs AlekseyTs requested review from a team as code owners February 22, 2024 01:48
@AlekseyTs
Copy link
Contributor Author

@RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review

@@ -345,7 +345,7 @@ private static ArgumentKind GetArgumentKind(BoundExpression argument, ref BitVec
}
else if (argument.IsParamsCollection)
{
argumentKind = ArgumentKind.ParamArray;
argumentKind = argument.Type?.IsSZArray() == true ? ArgumentKind.ParamArray : ArgumentKind.ParamCollection;
Copy link
Member

Choose a reason for hiding this comment

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

I thought that IsParamsCollection would only be true if it was actually a params collection, and IsParamsArray would be true in the other case, with IsParams being the catch-all. Is that just not implemented yet?

Copy link
Contributor Author

@AlekseyTs AlekseyTs Feb 22, 2024

Choose a reason for hiding this comment

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

I thought that IsParamsCollection would only be true if it was actually a params collection, and IsParamsArray would be true in the other case, with IsParams being the catch-all. Is that just not implemented yet?

This is not an IOperation API and is not a property on IParameterSymbol. Bound nodes do not have IsParamsArray or IsParams properties. The internal IsParamsCollection property covers both cases.

Copy link
Member

Choose a reason for hiding this comment

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

I see. It may be worth a refactoring to standardize them at some point then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may be worth a refactoring to standardize them at some point then.

I do not think it is needed.

DefaultValue = 0x3,

/// <summary>
/// Argument is a param collection created by compilers for the matching C# params y parameter.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what the y parameter at the end here is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what the y parameter at the end here is?

I guess the remainder artifact of "ParamArray"

@AlekseyTs AlekseyTs merged commit 05cf517 into dotnet:features/ParamsCollections Feb 22, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants