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

Reduce complexity of ParseStatementNoDeclaration #40290

Merged
merged 4 commits into from
Dec 13, 2019

Conversation

cston
Copy link
Member

@cston cston commented Dec 11, 2019

No description provided.

@cston cston requested a review from a team as a code owner December 11, 2019 04:58
@cston
Copy link
Member Author

cston commented Dec 11, 2019

cc @jaredpar, @agocke

@cston cston changed the base branch from master to release/dev16.4 December 11, 2019 05:33
@jaredpar
Copy link
Member

jaredpar commented Dec 11, 2019

Can we add a test that verifies the depth to which we can parse nested if statements now? Otherwise introducing a regression here seems pretty trivial. #Resolved

@RikkiGibson RikkiGibson added this to the 16.4 milestone Dec 11, 2019
Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 1)

@gafter
Copy link
Member

gafter commented Dec 11, 2019

I think we should consider making the parser more resilient to stack limitations by moving the work to a new stack/thread when nearing the limit of the current one (when attempting to parse a statement). The downside of such a change is that we may still run out of stack space in a later compiler phase.

@gafter
Copy link
Member

gafter commented Dec 11, 2019

I agree with @jaredpar that a test that prevents regression would be a very good idea... as a separate PR on master. #Resolved

@cston
Copy link
Member Author

cston commented Dec 11, 2019

I think we should consider making the parser more resilient to stack limitations by moving the work to a new stack/thread when nearing the limit of the current one

@gafter, agreed. Logged #40303.

@@ -6480,7 +6480,7 @@ private StatementSyntax ParseStatementNoDeclaration(bool allowAnyExpression)
}
else
{
return this.ParseLocalDeclarationStatement(parseAwaitKeyword());
return this.ParseLocalDeclarationStatement(parseAwaitKeyword(MessageID.None));
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, so the optional parameter prevented this from being inlined?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suspected it was the conversions to Nullable<MessageID> rather than the optional parameter. I decided to make the parameter non-optional at the same time.


In reply to: 356841763 [](ancestors = 356841763)

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

@cston cston merged commit 0ca43e5 into dotnet:release/dev16.4 Dec 13, 2019
@cston cston deleted the ParseStack branch December 13, 2019 17:59
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.

7 participants