-
Notifications
You must be signed in to change notification settings - Fork 4.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
Integrate Lambda Default Parameters into Synthesized Delegate Types and Lowering Pass #63293
Integrate Lambda Default Parameters into Synthesized Delegate Types and Lowering Pass #63293
Conversation
…date closure conversion pass + add tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done review pass (commit 7)
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedIntrinsicOperatorSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/AnonymousTypeField.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs
Outdated
Show resolved
Hide resolved
…ollow convention from source parameter symbols
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs
Outdated
Show resolved
Hide resolved
…tor to follow convention from source parameter symbols" This reverts commit 26375e1.
…Symbol in all cases for default values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done review pass. Just a couple of small things.
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs
Outdated
Show resolved
Hide resolved
…d test coverage for Caller{LineNumber, MemberName, FilePath} on lambda parameters with default values
src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs
Outdated
Show resolved
Hide resolved
Consider moving this after the if (constVal != null && parameterDefaultValueBuilder == null)
{
parameterDefaultValueBuilder = ... // allocate and fill
}
if (parameterDefaultValueBuilder != null)
{
parameterDefaultValueBuilder.Add(constVal);
} In reply to: 1213551892 Refers to: src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs:664 in 8254fca. [](commit_id = 8254fca, deletion_comment = False) |
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/SynthesizedMethodBaseSymbol.cs
Outdated
Show resolved
Hide resolved
… for synthesized parameter and default value for _baseParameterForAttributes
…odBaseSymbol.cs Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>
src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done review pass. Overall looking good, but test failure needs to be addressed.
src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 21), assuming tests are passing.
Allows default values to be attached to the parameters of synthesized delegate types and threaded through the closure lowering pass in order to support code-gen for lambda default parameters.
Still missing (will be added in a subsequent PR):