-
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
Simplify implementation of parsing constraints. #16429
Simplify implementation of parsing constraints. #16429
Conversation
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
list.Add(_syntaxFactory.SimpleBaseType(this.AddError(this.CreateMissingIdentifierName(), ErrorCode.ERR_TypeExpected))); | ||
} | ||
else | ||
{ | ||
TypeSyntax firstType = this.ParseDeclarationType(); |
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.
Indentation wrong on this line and two lines down.
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.
Fixed indentation.
@dotnet/roslyn-compiler Can i get a second pair of eyes? Thanks! |
Tagging @MattGertz Improvements in parser that benefit IDE and compiler. |
Approved pending tests. |
7c602ec
to
d2a0a89
Compare
Rebased to 15.6. |
@@ -1796,42 +1796,26 @@ private BaseListSyntax ParseBaseList() | |||
try | |||
{ | |||
// first type |
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.
If I understood correctly, the code that was removed (here and below) handled the case class C : where T : ...
and class C : I1, where T : ...
.
Do we have parser tests for those? If not, could you add them?
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.
Will check on this.
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.
This is parsed properly. That's because ParseType will not accept "where :" as a type name. I'm not sure if we have tests for this though, so i will add them.
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.
tests added.
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
…c-member-reference * dotnet/features/ioperation: (134 commits) Fix build break in IOperation feature branch Fix unit tests that broke with previous commit Address PR feedback from Chuck Fix unit test Add IOperation support for variations of object creation expressions (type parameter, nopia and dynamic) Remove dead code from GenerateConstructor. Disable NETCore version check Fixed review notes. Add comment Disable NETCore version check Show modifier/method/type block declaration keywords after attribute lists Improve type argument list parsing in error conditions. (dotnet#19467) Don't report modifier errors while parsing. (dotnet#16339) Fix assert and merge Remove extra space Restore failure behavior. Simplify implementation of parsing constraints. (dotnet#16429) Fix csproj Remove CapturedVariablesByLambda (dotnet#20878) Remove testing wait ...
No description provided.