Support built-in unsigned right shift operators. by AlekseyTs · Pull Request #60560 · dotnet/roslyn

AlekseyTs · 2022-04-04T18:46:24Z

https://github.com/dotnet/csharplang/blob/main/proposals/unsigned-right-shift-operator.md

Test plan #60433

https://github.com/dotnet/csharplang/blob/main/proposals/unsigned-right-shift-operator.md

AlekseyTs · 2022-04-04T21:57:27Z

@333fred, @jcouv, @dotnet/roslyn-compiler Please review.

333fred

Done review pass (commit 1)

333fred · 2022-04-04T23:02:31Z

src/Compilers/CSharp/Test/Symbol/Symbols/UnsignedRightShiftTests.cs

+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+#nullable disable


Did I miss a langver test in here? #Resolved

Did I miss a langver test in here?

I haven't touched the lang version part yet.

333fred · 2022-04-04T23:03:09Z

src/Compilers/CSharp/Portable/Binder/Semantics/Operators/OperatorKind.cs

+        LiftedULongUnsignedRightShift = Lifted | ULong | UnsignedRightShift,
+        LiftedNIntUnsignedRightShift = Lifted | NInt | UnsignedRightShift,
+        LiftedNUIntUnsignedRightShift = Lifted | NUInt | UnsignedRightShift,
+        LiftedUserDefinedUnsignedRightShift = Lifted | UserDefined | UnsignedRightShift,


It didn't seem like this was tested, should it also be commented out with a prototype for now? #Resolved

It didn't seem like this was tested, should it also be commented out with a prototype for now?

I do not think it is strictly necessary. Working on user-defined operators now (for the next PR)

Alright. I don't have a problem with it, was just curious since you commented out the non-lifted variant.

AlekseyTs · 2022-04-05T16:46:20Z

@jcouv, @dotnet/roslyn-compiler For the second review

AlekseyTs · 2022-04-05T22:14:06Z

@jcouv, @dotnet/roslyn-compiler For the second review

AlekseyTs · 2022-04-06T13:55:29Z

@jcouv, @dotnet/roslyn-compiler For the second review

AlekseyTs · 2022-04-06T20:46:31Z

@jcouv, @dotnet/roslyn-compiler For the second review

jcouv · 2022-04-06T22:17:57Z

src/Compilers/CSharp/Portable/CodeGen/EmitOperators.cs

                    }
                    break;

+                case BinaryOperatorKind.UnsignedRightShift:


Consider adding an assertion in IsUnsignedBinaryOperator that it's not meant to be used on an unsigned right shift. #Resolved

Consider adding an assertion in IsUnsignedBinaryOperator that it's not meant to be used on an unsigned right shift.

Will add in one of the next PRs

jcouv · 2022-04-06T22:28:34Z

src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs

+                case BinaryOperatorKind.IntUnsignedRightShift:
+                    return (int)(((uint)valueLeft.Int32Value) >> valueRight.Int32Value); // Switch to `valueLeft.Int32Value >>> valueRight.Int32Value` once >>> becomes available
+                case BinaryOperatorKind.NIntUnsignedRightShift:
+                    return (valueLeft.Int32Value >= 0) ? valueLeft.Int32Value >> valueRight.Int32Value : null;


I didn't understand why we can't compute the constant result when valueLeft.Int32Value < 0 #Closed

I didn't understand why we can't compute the constant result when valueLeft.Int32Value < 0

Binary representation of negative values depends on the actual size of the type.

Worked it out on paper. Makes sense

jcouv · 2022-04-06T22:28:37Z

src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs

                case BinaryOperatorKind.NIntRightShift:
                    return valueLeft.Int32Value >> valueRight.Int32Value;
+                case BinaryOperatorKind.IntUnsignedRightShift:
+                    return (int)(((uint)valueLeft.Int32Value) >> valueRight.Int32Value); // Switch to `valueLeft.Int32Value >>> valueRight.Int32Value` once >>> becomes available


nit: Consider opening a tracking issue, otherwise we'll likely forget

jcouv · 2022-04-06T22:44:20Z

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IBinaryOperatorExpression.cs


+        [CompilerTrait(CompilerFeature.IOperation)]
+        [Fact]
+        public void TestBinaryOperators_UnsignedRightShift()


Should we also have IOp tests with >>>=, or will that be a later PR? #Resolved

Should we also have IOp tests with >>>=, or will that be a later PR?

Yes we should have those. I will add them in a later PR.

jcouv · 2022-04-06T22:55:11Z

src/Compilers/CSharp/Test/Symbol/Symbols/UnsignedRightShiftTests.cs

+        [InlineData("decimal", "nuint")]
+        [InlineData("decimal", "float")]
+        [InlineData("decimal", "double")]
+        [InlineData("decimal", "decimal")]


Consider using CombinatorialData (more compact, less tedious to type/review). This applies to other tests as well #Resolved

Consider using CombinatorialData (more compact, less tedious to type/review). This applies to other tests as well

Will simplify in one of the next PRs.

Actually, I do not think CombinatorialData is going to work. We are not testing cross product of two type sets.

jcouv · 2022-04-06T22:57:53Z

src/Compilers/CSharp/Test/Symbol/Symbols/UnsignedRightShiftTests.cs

+}
+";
+            var compilation1 = CreateCompilation(source1, options: TestOptions.DebugDll,
+                                                 parseOptions: TestOptions.RegularPreview);


nit: all these tests don't need TestOptions.RegularPreview (that's the default) #WontFix

jcouv · 2022-04-06T22:59:28Z

src/Compilers/CSharp/Test/Symbol/Symbols/UnsignedRightShiftTests.cs

+            Assert.Equal("x >>> y", unsignedShift.ToString());
+            Assert.Equal("x >> y", shift.ToString());
+
+            var unsignedShiftSymbol = (IMethodSymbol)model.GetSymbolInfo(unsignedShift).Symbol;


Consider verifying GetTypeInfo too, or adding to test plan #WontFix

Consider verifying GetTypeInfo too, or adding to test plan

I do not find this particularly interesting. Completeness is the only reason to have them. I am not adding new bound nodes, there is nothing new there in terms of calculating type info. Symbol info is interesting because we are supposed to synthesize operator symbols for intrinsic operators.

jcouv

LGTM Thanks (iteration 1), but reducing the verbosity of the InlineData sequences in the tests would be really appreciated. Also a couple of nits to consider

Support built-in unsigned right shift operators.

80b5da9

https://github.com/dotnet/csharplang/blob/main/proposals/unsigned-right-shift-operator.md

AlekseyTs added Area-Compilers Feature - UnsignedRightShift labels Apr 4, 2022

AlekseyTs requested review from a team, 333fred and jcouv April 4, 2022 18:46

AlekseyTs requested a review from a team as a code owner April 4, 2022 18:46

333fred reviewed Apr 4, 2022

View reviewed changes

333fred approved these changes Apr 4, 2022

View reviewed changes

jcouv self-assigned this Apr 5, 2022

jcouv reviewed Apr 6, 2022

View reviewed changes

jcouv approved these changes Apr 6, 2022

View reviewed changes

AlekseyTs merged commit c7b2b5a into dotnet:features/UnsignedRightShift Apr 7, 2022

Conversation

AlekseyTs commented Apr 4, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Apr 4, 2022

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

333fred Apr 4, 2022 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Apr 4, 2022 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Apr 5, 2022

Uh oh!

AlekseyTs commented Apr 5, 2022

Uh oh!

AlekseyTs commented Apr 6, 2022

Uh oh!

AlekseyTs commented Apr 6, 2022

Uh oh!

jcouv Apr 6, 2022 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 6, 2022 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 6, 2022 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 6, 2022 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 6, 2022 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

AlekseyTs commented Apr 4, 2022 •

edited by jcouv

Loading

333fred Apr 4, 2022 •

edited by AlekseyTs

Loading

333fred Apr 4, 2022 •

edited by AlekseyTs

Loading

jcouv Apr 6, 2022 •

edited by AlekseyTs

Loading

jcouv Apr 6, 2022 •

edited

Loading

jcouv Apr 6, 2022 •

edited by AlekseyTs

Loading

jcouv Apr 6, 2022 •

edited by AlekseyTs

Loading

jcouv Apr 6, 2022 •

edited by AlekseyTs

Loading

jcouv Apr 6, 2022 •

edited by AlekseyTs

Loading