Skip to content

Fix stack overflow with large chained expressions.#49096

Merged
3 commits merged intodotnet:masterfrom
CyrusNajmabadi:stackOverflow
Oct 31, 2020
Merged

Fix stack overflow with large chained expressions.#49096
3 commits merged intodotnet:masterfrom
CyrusNajmabadi:stackOverflow

Conversation

@CyrusNajmabadi
Copy link
Contributor

No description provided.

Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Nit: no point having the ArrayBuilder passed in now, could just be internal to that method.

@ghost
Copy link

ghost commented Oct 31, 2020

Hello @CyrusNajmabadi!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@ghost ghost merged commit 17aab57 into dotnet:master Oct 31, 2020
@ghost ghost added this to the Next milestone Oct 31, 2020
@jamiehankins
Copy link

@CyrusNajmabadi this is where knowing the codebase helps.

The reason I did the dual-state thing is because you get in a SyntaxNode as the root, which can either, but I didn't know how to go from SyntaxToken to SyntaxNodeOrToken, so I couldn't easily push the top node to the stack, hence the other logic. I tried using the constructor "SyntaxNodeOrToken(SyntaxNode node)", but Intellisense was complaining that its protection level didn't allow me to call it. Now I see that was probably an Intellisense bug.

Thanks for finishing this up.

There is still another stack recursion bug. It's a bit more complicated, since it involves three methods. At least with it, it's handled and there's a warning at the top instead of the whole IDE crashing.

@CyrusNajmabadi CyrusNajmabadi deleted the stackOverflow branch October 31, 2020 20:09
@CyrusNajmabadi
Copy link
Contributor Author

CyrusNajmabadi commented Oct 31, 2020

Now I see that was probably an Intellisense bug.

There's no intellisense issue there. The constructor isn't publicly accessible. Instead, there is an implicit conversion. It's a way of simulating inheritance with structs.

This pull request was closed.
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.

5 participants