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

Properly detect root of a tree in Operation.SearchParentOperation #22974

Merged
merged 1 commit into from
Nov 3, 2017

Conversation

AlekseyTs
Copy link
Contributor

@AlekseyTs AlekseyTs commented Nov 2, 2017

@AlekseyTs AlekseyTs added Area-Compilers Feature - IOperation IOperation PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Nov 2, 2017
@AlekseyTs AlekseyTs requested review from heejaechang, a team and cston November 2, 2017 16:40
@AlekseyTs AlekseyTs added this to the 15.5 milestone Nov 2, 2017
@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler, @dotnet/analyzer-ioperation, @heejaechang, @cston Please review 15.5 IOperation fix.

Copy link
Member

@genlu genlu left a comment

Choose a reason for hiding this comment

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

:shipit:

@@ -286,6 +285,12 @@ internal IOperation SearchParentOperation()
}
}

if (SemanticModel.Root == currentCandidate)
Copy link
Member

Choose a reason for hiding this comment

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

I believe you could avoid duplicating this check by doing it at the beginning of the loop and setting currentCandidate to Syntax at the beginning of the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe you could avoid duplicating this check by doing it at the beginning of the loop and setting currentCandidate to Syntax at the beginning of the loop.

Then the loop would have to be changed to ```while (true) and the null check moved inside. I believe the condition is simple enough that duplication is not an issue.

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

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM

@jinujoseph
Copy link
Contributor

Pre-Approved to merge via Link

@AlekseyTs AlekseyTs merged commit 289aebf into dotnet:master Nov 3, 2017
333fred added a commit to 333fred/roslyn that referenced this pull request Nov 8, 2017
* dotnet/master: (36 commits)
  Remove usage of IOperation feature flag in newly added unit tests
  More fixes for IOperation tree. (dotnet#23028)
  Address PR feedback and remove unused resource and comment out unused error code
  Remove IOperation feature flag and internal registration APIs for operations
  Add unit test for VB
  Address review feedback and add more unit tests
  Fix unit tests
  Fix build errors from merge resolution
  Revert unintentional newline deletion
  Address recent feedback and do not generate an IParenthesizedOperation for C#. This change is now just a test-only change that verifies the current behavior.
  Address PR feedback and use singleton for null instance
  Address PR feedback
  Address PR feedback
  Do not invoke GetStandaloneNode helper in GetOperationWorker
  Fix InvalidCastException in CSharpOperationFactory for invalid nested member initializer (dotnet#22983)
  Use ICollection<string>.Contains to avoid Enumerator allocation
  Add IsRef property to IConditionalOperation and ISimpleAssignmentOperation, add RefKind property to ILocalSymbol. (dotnet#22933)
  Enable peverify-compat mode for langver < 7.2 (dotnet#22772)
  Properly detect root of a tree in Operation.SearchParentOperation (dotnet#22974)
  Fix IArgument and IArrayInitializer to have null types
  ...
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