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

Make '^' operation nodes internal #32918

Merged
merged 2 commits into from
Feb 6, 2019

Conversation

agocke
Copy link
Member

@agocke agocke commented Jan 29, 2019

We plan to mark this as a non-user overridable unary operator in a later release, so this node should not be in the public API.

@agocke agocke added this to the 16.0.P4 milestone Jan 29, 2019
@agocke agocke requested a review from a team January 29, 2019 21:22
@agocke
Copy link
Member Author

agocke commented Jan 29, 2019

FYI @jaredpar for shiproom

@jcouv
Copy link
Member

jcouv commented Jan 29, 2019

Looks like some tests need to be updated:

2019-01-29T22:23:10.0853646Z     Microsoft.CodeAnalysis.CSharp.UnitTests.IOperationTests.VerifyIndexOperator_Int [FAIL]
2019-01-29T22:23:10.0854336Z     Microsoft.CodeAnalysis.CSharp.UnitTests.IOperationTests.VerifyRangeOperator_NullableInt_Create_WithHat [FAIL]
2019-01-29T22:23:10.0854812Z     Microsoft.CodeAnalysis.CSharp.UnitTests.IOperationTests.FromEndIndexFlow_01 [FAIL]
2019-01-29T22:23:10.0855082Z     Microsoft.CodeAnalysis.CSharp.UnitTests.IOperationTests.VerifyIndexOperator_NullableInt [FAIL]
2019-01-29T22:23:10.0855193Z     Microsoft.CodeAnalysis.CSharp.UnitTests.IOperationTests.VerifyIndexOperator_ConvertibleToInt [FAIL]
2019-01-29T22:23:10.0855360Z     Microsoft.CodeAnalysis.CSharp.UnitTests.IOperationTests.VerifyRangeOperator_Int_Create_WithHat [FAIL]

@@ -226,8 +226,6 @@ public enum OperationKind
SuppressNullableWarning = 0x62,
/// <summary>Indicates an <see cref="IRangeOperation"/>.</summary>
Range = 0x63,
/// <summary>Indicates an <see cref="IFromEndIndexOperation"/>.</summary>
FromEndIndex = 0x64,
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 30, 2019

Choose a reason for hiding this comment

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

FromEndIndex = 0x64, [](start = 8, length = 20)

Perhaps keep this as a comment, or add a comment indicating that the value is available for reuse #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 30, 2019

Done with review pass (iteration 1) #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 30, 2019

Please make sure this change does not introduce new failures when tests are run with IOperation hook enabled. #Closed

@agocke
Copy link
Member Author

agocke commented Feb 5, 2019

@AlekseyTs Done. I ran build -testIOperation and I saw a number of failures, but it doesn't look like this introduces any new ones.

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 2)

@agocke
Copy link
Member Author

agocke commented Feb 5, 2019

@jaredpar Can we take this PR to make the IFromEndOperation nodes internal in preview 4? These nodes will be removed before RTM (#33155)

@jaredpar
Copy link
Member

jaredpar commented Feb 6, 2019

Yes but get it in soon. We're snappnig I imagine tomorrow night.

@agocke
Copy link
Member Author

agocke commented Feb 6, 2019

@jaredpar This is ready, if I'm good to merge

@agocke agocke merged commit 19623b8 into dotnet:master Feb 6, 2019
@agocke agocke deleted the remove-ifromendindex-ioperation branch February 6, 2019 23:47
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