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

Support parsing of checked user-defined operator declarations #59504

Conversation

AlekseyTs
Copy link
Contributor

@AlekseyTs AlekseyTs commented Feb 11, 2022

https://github.com/dotnet/csharplang/blob/main/proposals/checked-user-defined-operators.md

Language version checks and whether checked is applicable to specific operator will be checked during declaration binding and is out of scope for this PR.

Relates to test plan #59458

@@ -3305,8 +3305,23 @@ private ConversionOperatorDeclarationSyntax TryParseConversionOperatorDeclaratio

if (style.IsMissing)
{
if (this.CurrentToken.Kind != SyntaxKind.OperatorKeyword || SyntaxFacts.IsAnyOverloadableOperator(this.PeekToken(1).Kind) ||
bool possibleConversion = true;
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 11, 2022

Choose a reason for hiding this comment

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

nit: remove initial value. it is never observed. #Resolved

@CyrusNajmabadi
Copy link
Member

NIt: consider detecting, reporting, and gracefully recovering from errant use of unchecked in the location where checked is used. I can see people easily trying that out and it would potentially be good to have the experience gracefully handle that.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Grammar/Parser changes LGTM.

@@ -2514,6 +2514,33 @@ public static BlockSyntax Block(IEnumerable<StatementSyntax> statements)
expressionBody: expressionBody);
}

/// <summary>Creates a new ConversionOperatorDeclarationSyntax instance.</summary>
Copy link
Member

@Youssef1313 Youssef1313 Feb 12, 2022

Choose a reason for hiding this comment

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

nit: <see cref="..." /> (applies to below methods as well) #Resolved

@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review

@AlekseyTs
Copy link
Contributor Author

@333fred, @chsienki, @dotnet/roslyn-compiler Please review.

Copy link
Contributor

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

LGTM

@AlekseyTs
Copy link
Contributor Author

@333fred, @dotnet/roslyn-compiler For the second review.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 1). You could address Cyrus's comment about unnecessary local initialization in LanguageParser in follow-up PR

@jcouv jcouv self-assigned this Feb 15, 2022
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 1)

@AlekseyTs AlekseyTs enabled auto-merge (squash) February 15, 2022 23:14
@AlekseyTs
Copy link
Contributor Author

azp /run

@AlekseyTs
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@AlekseyTs AlekseyTs merged commit 25c6a00 into dotnet:features/CheckedUserDefinedOperators Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants