-
Notifications
You must be signed in to change notification settings - Fork 4k
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 handling of params collections for natural delegate types #71334
Add handling of params collections for natural delegate types #71334
Conversation
@333fred, @dotnet/roslyn-compiler Please review. |
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.
src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs
Outdated
Show resolved
Hide resolved
@333fred Please take another look. |
@333fred, @dotnet/roslyn-compiler Please review. |
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.
Minor grammar comment, otherwise LGTM.
...s/CSharp/Portable/Symbols/AnonymousTypes/PublicSymbols/AnonymousType.DelegatePublicSymbol.cs
Outdated
Show resolved
Hide resolved
…bols/AnonymousType.DelegatePublicSymbol.cs Co-authored-by: Fred Silberberg <fred@silberberg.xyz>
@dotnet/roslyn-compiler For the second review. |
4 similar comments
@dotnet/roslyn-compiler For the second review. |
@dotnet/roslyn-compiler For the second review. |
@dotnet/roslyn-compiler For the second review. |
@dotnet/roslyn-compiler For the second review. |
@RikkiGibson, @dotnet/roslyn-compiler For the second review |
Is this meant to address #71333? |
No. This is an unrelated change |
@RikkiGibson, @dotnet/roslyn-compiler For the second review |
src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs
Outdated
Show resolved
Hide resolved
var src1 = @" | ||
public class Params | ||
{ | ||
static public void Test1(params System.ReadOnlySpan<long> a) {} |
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.
Do we have any test for a scenario like the following? Would such a test be useful? No need to take any action in this PR, just want to have the test plan item written down if applicable.
// comp1
public delegate void Del(params IEnumerable<int> e);
public static void Test1(Del del) { }
// comp2
public static void Main()
{
Test1(e => { });
}
``` #Pending
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.
Would such a test be useful?
During an offline conversation we arrived to the answer that it would be interesting to verify whether any language version diagnostics going to be reported on the Test1
invocation in the second compilation. Since the scenario is not primarily related to the changes made in this PR, I am not going to add a test like this here. But, I will incorporate it to one of my next PRs.
…s.cs Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>
No description provided.