Adding more tests for the generic math feature #55377

tannergooding · 2021-07-08T23:30:11Z

This adds more tests covering the generic math feature for integer types.

CC. @pgovind, @stephentoub, @jeffhandley

dotnet-issue-labeler · 2021-07-08T23:30:17Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

dotnet-issue-labeler · 2021-07-08T23:30:18Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

src/libraries/System.Private.CoreLib/src/System/Byte.cs

stephentoub · 2021-07-09T01:36:47Z

src/libraries/System.Private.CoreLib/src/System/Byte.cs

@@ -313,11 +313,11 @@ static byte IBinaryInteger<byte>.PopCount(byte value)
            => (byte)BitOperations.PopCount(value);

        [RequiresPreviewFeatures]
-        static byte IBinaryInteger<byte>.RotateLeft(byte value, byte rotateAmount)
+        static byte IBinaryInteger<byte>.RotateLeft(byte value, int rotateAmount)


This was a mistake in the interface definition?

Yesn't.

Ideally both rotate and shift operators would take TSelf on the RHS. However, shift operators require dotnet/csharplang#4666 to go in and so Rotate should match until it does.

stephentoub · 2021-07-09T01:38:18Z

src/libraries/System.Private.CoreLib/src/System/SByte.cs

@@ -318,19 +318,19 @@ object IConvertible.ToType(Type type, IFormatProvider? provider)

        [RequiresPreviewFeatures]
        static sbyte IBinaryInteger<sbyte>.LeadingZeroCount(sbyte value)
-            => (sbyte)(BitOperations.LeadingZeroCount((byte)value) - 16);
+            => (sbyte)(BitOperations.LeadingZeroCount((byte)value) - 24);


Yay for tests

stephentoub · 2021-07-09T13:59:38Z

src/libraries/System.Runtime/tests/System/ByteTests.GenericMath.cs

+        [Fact]
+        public static void TryCreateFromCharTest()
+        {
+            byte result;
+
+            Assert.True(NumberHelper<byte>.TryCreate<char>((char)0x0000, out result));
+            Assert.Equal((byte)0x00, result);
+
+            Assert.True(NumberHelper<byte>.TryCreate<char>((char)0x0001, out result));
+            Assert.Equal((byte)0x01, result);
+
+            Assert.False(NumberHelper<byte>.TryCreate<char>((char)0x7FFF, out result));
+            Assert.Equal((byte)0x00, result);
+
+            Assert.False(NumberHelper<byte>.TryCreate<char>((char)0x8000, out result));
+            Assert.Equal((byte)0x00, result);
+
+            Assert.False(NumberHelper<byte>.TryCreate<char>((char)0xFFFF, out result));
+            Assert.Equal((byte)0x00, result);
+        }


Did you consider using theories to tighten up these tests and avoid the duplication? e.g. I prefer the conciseness and clarity of this:

Suggested change

[Fact]

public static void TryCreateFromCharTest()

{

byte result;

Assert.True(NumberHelper<byte>.TryCreate<char>((char)0x0000, out result));

Assert.Equal((byte)0x00, result);

Assert.True(NumberHelper<byte>.TryCreate<char>((char)0x0001, out result));

Assert.Equal((byte)0x01, result);

Assert.False(NumberHelper<byte>.TryCreate<char>((char)0x7FFF, out result));

Assert.Equal((byte)0x00, result);

Assert.False(NumberHelper<byte>.TryCreate<char>((char)0x8000, out result));

Assert.Equal((byte)0x00, result);

Assert.False(NumberHelper<byte>.TryCreate<char>((char)0xFFFF, out result));

Assert.Equal((byte)0x00, result);

}

[Theory]

[InlineData(0x0000, 0x00)]

[InlineData(0x0001, 0x01)]

[InlineData(0x7FFF, 0x00)]

[InlineData(0x8000, 0x00)]

[InlineData(0xFFFF, 0x00)]

public static void TryCreateFromCharTest(int input, int expected)

{

Assert.True(NumberHelper<byte>.TryCreate<char>((char)input, out byte result));

Assert.Equal((byte)expected, result);

}

I'll log an issue for this as a low hanging fruit to make these easier to manage and add new cases for in the future.

src/libraries/System.Runtime/tests/System/CharTests.GenericMath.cs

stephentoub · 2021-07-09T14:01:59Z

src/libraries/System.Runtime/tests/System/GenericMathTests.cs

-            return TResult.Create(sum) / TResult.Create(values.Count());
-        }
-
-        public static TResult StandardDeviation<TSelf, TResult>(IEnumerable<TSelf> values)


Why delete these?

Accident. Must have deleted these by mistake when adding in the other tests.

stephentoub

Spot checked and left a few comments. Thanks.

lambdageek · 2021-07-09T14:50:21Z

mono interp failure looks relevant
* Assertion: should not be reached at /__w/1/s/src/mono/mono/mini/interp/transform.c:5328 is an unexpected STACK_TYPE_ in a CEE_CONV_U instruction /cc @BrzVlad

BrzVlad · 2021-07-09T22:06:53Z

mono interp failures should have been fixed in #55418

tannergooding · 2021-07-09T23:38:47Z

Rebased to pick up fixes.

pgovind · 2021-07-13T17:18:46Z

src/libraries/System.Runtime/tests/System/ByteTests.GenericMath.cs

+        [Fact]
+        public static void MinTest()
+        {
+            Assert.Equal((byte)0x00, NumberHelper<byte>.Min((byte)0x00, (byte)1));


I feel like Theory would be easier here, but it's a nit, so I'm not too bothered. I also don't know how you generated these. If you used a tt file, I'm sure writing a Fact was easier

Oh never mind, I saw your reply to Toub

pgovind

I checked a few places and everything looked good. (I know it's already merged :))

dotnet-issue-labeler bot added the new-api-needs-documentation label Jul 8, 2021

stephentoub reviewed Jul 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Byte.cs Show resolved Hide resolved

stephentoub reviewed Jul 9, 2021

View reviewed changes

src/libraries/System.Runtime/tests/System/CharTests.GenericMath.cs Show resolved Hide resolved

stephentoub reviewed Jul 9, 2021

View reviewed changes

stephentoub approved these changes Jul 9, 2021

View reviewed changes

tannergooding added 2 commits July 9, 2021 16:38

Fixing some issues in the generic math implementation

9d8b4e2

Adding generic math tests for integer types

0420af8

tannergooding force-pushed the generic-math branch from fe4fc18 to 0420af8 Compare July 9, 2021 23:38

tannergooding merged commit bb39822 into dotnet:main Jul 11, 2021

pgovind reviewed Jul 13, 2021

View reviewed changes

ManickaP mentioned this pull request Jul 20, 2021

[QUIC] Remove AppContext switch from S.N.Quic #56027

Merged

ghost locked as resolved and limited conversation to collaborators Aug 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding more tests for the generic math feature #55377

Adding more tests for the generic math feature #55377

tannergooding commented Jul 8, 2021

dotnet-issue-labeler bot commented Jul 8, 2021

dotnet-issue-labeler bot commented Jul 8, 2021

stephentoub Jul 9, 2021

tannergooding Jul 9, 2021

stephentoub Jul 9, 2021

stephentoub Jul 9, 2021

tannergooding Jul 9, 2021

stephentoub Jul 9, 2021

tannergooding Jul 12, 2021

stephentoub left a comment

lambdageek commented Jul 9, 2021

BrzVlad commented Jul 9, 2021

tannergooding commented Jul 9, 2021

pgovind Jul 13, 2021

pgovind Jul 13, 2021

pgovind left a comment

Adding more tests for the generic math feature #55377

Adding more tests for the generic math feature #55377

Conversation

tannergooding commented Jul 8, 2021

dotnet-issue-labeler bot commented Jul 8, 2021

dotnet-issue-labeler bot commented Jul 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

lambdageek commented Jul 9, 2021

BrzVlad commented Jul 9, 2021

tannergooding commented Jul 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgovind left a comment

Choose a reason for hiding this comment