-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Initial support for conditional logical binary extension operators #78706
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
base: features/extensions
Are you sure you want to change the base?
Conversation
@jcouv, @dotnet/roslyn-compiler Please review |
|
||
[Theory] | ||
[CombinatorialData] | ||
public void Binary_045_Consumption_Logical_InDifferentBlocks([CombinatorialValues("&&", "||")] string op) |
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.
nit: Consider testing from metadata too #Closed
|
||
[Theory] | ||
[CombinatorialData] | ||
public void Binary_053_Consumption_Logical_TrueOrFalseInDifferentClass([CombinatorialValues("&&", "||")] string op) |
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 a test like this but the true/false operators are together (separate from the logical operator)? #Closed
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.
I do not find this scenario particularly interesting. Adding Binary_055_Consumption_Logical_TrueOrFalseInDifferentClass for completeness.
static void Main() | ||
{ | ||
var s1 = (1, 2); | ||
s1 = s1 {{{op}}} s1; |
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.
Consider verifying the info from semantic model in scenario with "generic operator" #Closed
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 Thanks (commit 1) with some test suggestions
The title of the PR seems off. The logical operators are |
"""; | ||
|
||
var comp = CreateCompilation(src1, options: TestOptions.DebugExe); | ||
comp.VerifyDiagnostics( |
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.
VerifyEmitDiagnostics? (here and elsewhere as well) #Closed
@dotnet/roslyn-compiler For the second review. |
@dotnet/roslyn-compiler For another 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.
LGTM Thanks (commit 2)
@dotnet/roslyn-compiler For another review |
@dotnet/roslyn-compiler For another review |
Relates to test plan #76130