Create params arrays during binding#70762
Conversation
|
@dotnet/roslyn-compiler Please review. |
1 similar comment
|
@dotnet/roslyn-compiler Please review. |
| { | ||
| internal partial class BoundArrayCreation | ||
| { | ||
| public new bool IsParamsArray |
There was a problem hiding this comment.
What is the effect of declaring a 'new' member in this type versus using the member in the base type? #Resolved
There was a problem hiding this comment.
What is the effect of declaring a 'new' member in this type versus using the member in the base type?
The setter in the base type is intentionally protect. The new property allows setting it for BoundArrayCreation from outside the type.
| // Params methods can be invoked in normal form, so the strongest assertion we can make is that, if | ||
| // we're in an expanded context, the last param must be params. The inverse is not necessarily true. | ||
| Debug.Assert(!expanded || parameters[^1].IsParams); | ||
| // Params array is filled in the local rewriter |
There was a problem hiding this comment.
This comment should probably be deleted #Resolved
| private ImmutableArray<TypedConstant> GetRewrittenAttributeConstructorArguments( | ||
| MethodSymbol attributeConstructor, | ||
| ImmutableArray<TypedConstant> constructorArgsArray, | ||
| ImmutableArray<string?> constructorArgumentNamesOpt, |
There was a problem hiding this comment.
Consider also deleting the <remarks> for this method. #Resolved
| { | ||
| reorderedArgument = constructorArgsArray[i]; | ||
| } | ||
| TypedConstant reorderedArgument = reorderedArgument = constructorArgsArray[i]; |
There was a problem hiding this comment.
It looks like we unintentionally assigned twice to this variable. #Resolved
| /// arguments based on <paramref name="argsToParamsOpt"/> map, inserting arguments for optional parameters, etc. | ||
| /// </summary> | ||
| private ImmutableArray<BoundExpression> MakeArguments( | ||
| SyntaxNode syntax, |
There was a problem hiding this comment.
Doc comment could also be adjusted to remove "generating a params array". #Resolved
| ImmutableArray<ParameterSymbol> parameters = methodOrIndexer.GetParameters(); | ||
| BoundExpression? optimized; | ||
|
|
||
| Debug.Assert(expanded ? rewrittenArguments.Length == parameters.Length : rewrittenArguments.Length >= parameters.Length); |
There was a problem hiding this comment.
Is it only in arglist scenarios where expanded is false and rewrittenArguments.Length > parameters.Length? #Resolved
There was a problem hiding this comment.
Is it only in arglist scenarios where expanded ...
I think so.
|
@dotnet/roslyn-compiler For the second review. |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System.Diagnostics; |
| } | ||
|
|
||
| arrayArgsBuilder.Add(argumentsBuilder[remainingArgument]); | ||
| i++; |
There was a problem hiding this comment.
Looks like we could use i in this loop directly instead of remainingArgument since we are incrementing it here anyway. #Resolved
There was a problem hiding this comment.
Looks like we could use
iin this loop directly instead ofremainingArgumentsince we are incrementing it here anyway.
I do not think so. The i is incremented conditionally.
| return default(ImmutableArray<RefKind>); | ||
| } | ||
|
|
||
| private delegate BoundExpression ParamsArrayElementRewriter<TArg>(BoundExpression element, ref TArg arg); |
There was a problem hiding this comment.
Consider using in for arg instead. #Resolved
There was a problem hiding this comment.
Consider using
infor arg instead.
Ref is intentional, the argument is not read-only.
| getEnumeratorInfo = new MethodArgumentInfo( | ||
| getEnumeratorInfo.Method, | ||
| builder.ToImmutableAndFree(), | ||
| defaultArguments: default, |
There was a problem hiding this comment.
Shouldn't we pass getEnumeratorInfo.DefaultArguments here? #Resolved
There was a problem hiding this comment.
Shouldn't we pass
getEnumeratorInfo.DefaultArgumentshere?
Values in getEnumeratorInfo.DefaultArguments are obsolete since it is a map for the original argument list and we mutated it. We could, of course, adjust the map as well, but it will be a throw away effort because the map is not used for lowering, the arguments are already in place.
Closes #49602.