close
Skip to content

Fix module-level SkipLocalsInit with top-level statements#49435

Merged
RikkiGibson merged 1 commit into
dotnet:masterfrom
PathogenDavid:fix-49434
Nov 18, 2020
Merged

Fix module-level SkipLocalsInit with top-level statements#49435
RikkiGibson merged 1 commit into
dotnet:masterfrom
PathogenDavid:fix-49434

Conversation

@PathogenDavid
Copy link
Copy Markdown
Contributor

@PathogenDavid PathogenDavid commented Nov 17, 2020

Fixes #49434

@PathogenDavid PathogenDavid requested a review from a team as a code owner November 17, 2020 09:59
@PathogenDavid PathogenDavid mentioned this pull request Nov 17, 2020
Closed
@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Nov 17, 2020

Done with review pass (iteration 1)

@PathogenDavid
Copy link
Copy Markdown
Contributor Author

PathogenDavid commented Nov 17, 2020

Just pushed new revision rebased on master with the suggested changes.

@PathogenDavid PathogenDavid force-pushed the fix-49434 branch 2 times, most recently from cc5b8d3 to 99b1f7c Compare November 17, 2020 23:23
@PathogenDavid
Copy link
Copy Markdown
Contributor Author

PathogenDavid commented Nov 18, 2020 •
edited
Loading

I believe the failing test (Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Squiggles.ErrorSquiggleProducerTests.TestNoErrorsAfterProjectRemoved) is just being flakey since it passed on my machine and on the other build configurations.

Closing and reopening to (hopefully) re-run tests.

Edit: Yup, that did the trick. Should the test be reported or is it assumed that RunfoApp will report it if it continues to be a problem?

@PathogenDavid PathogenDavid reopened this Nov 18, 2020
@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Nov 18, 2020

Just pushed new revision rebased on master with the suggested changes.

In the future, please, do not rewrite commits in PRs under review. Simply push new commits, this allows us to do an incremental review, which often takes less time. We can squash commits when PR is merged.

@PathogenDavid
Copy link
Copy Markdown
Contributor Author

PathogenDavid commented Nov 18, 2020

Got it, sorry for the inconvenience.

AlekseyTs
AlekseyTs approved these changes Nov 18, 2020
View reviewed changes
Copy link
Copy Markdown
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)

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Nov 18, 2020

@RikkiGibson Please review the latest revision.

@jaredpar jaredpar added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Nov 18, 2020
@RikkiGibson RikkiGibson merged commit 198c614 into dotnet:master Nov 18, 2020
@ghost ghost added this to the Next milestone Nov 18, 2020
@RikkiGibson
Copy link
Copy Markdown
Member

RikkiGibson commented Nov 18, 2020

Thanks @PathogenDavid!

@PathogenDavid
Copy link
Copy Markdown
Contributor Author

PathogenDavid commented Nov 19, 2020

No problem, thank you both for the reviews and feedback!

@allisonchou allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020
@mikadumont mikadumont mentioned this pull request Dec 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees

No one assigned

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Milestone

16.9.P2

Development

Successfully merging this pull request may close these issues.

Module-level SkipLocalsInit does not apply to synthesized Main for top-level statements

6 participants

@PathogenDavid @AlekseyTs @RikkiGibson @jaredpar @allisonchou @Dotnet-GitSync-Bot