Adding more tests for the generic math feature by tannergooding · Pull Request #55377 · dotnet/runtime

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

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

CC. @pgovind, @stephentoub, @jeffhandley

ghost · 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.

ghost · 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.

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

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

Yay for tests

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

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]
publicstaticvoidTryCreateFromCharTest()
{
byteresult;
Assert.True(NumberHelper<byte>.TryCreate<char>((char)0x0000,outresult));
Assert.Equal((byte)0x00,result);
Assert.True(NumberHelper<byte>.TryCreate<char>((char)0x0001,outresult));
Assert.Equal((byte)0x01,result);
Assert.False(NumberHelper<byte>.TryCreate<char>((char)0x7FFF,outresult));
Assert.Equal((byte)0x00,result);
Assert.False(NumberHelper<byte>.TryCreate<char>((char)0x8000,outresult));
Assert.Equal((byte)0x00,result);
Assert.False(NumberHelper<byte>.TryCreate<char>((char)0xFFFF,outresult));
Assert.Equal((byte)0x00,result);
}
[Theory]
[InlineData(0x0000,0x00)]
[InlineData(0x0001,0x01)]
[InlineData(0x7FFF,0x00)]
[InlineData(0x8000,0x00)]
[InlineData(0xFFFF,0x00)]
publicstaticvoidTryCreateFromCharTest(intinput,intexpected)
{
Assert.True(NumberHelper<byte>.TryCreate<char>((char)input,outbyteresult));
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.

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

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

+ [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 :))

ghost added the new-api-needs-documentation label Jul 8, 2021

stephentoub reviewed Jul 9, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Byte.cs Outdated

stephentoub reviewed Jul 9, 2021
View reviewed changes

Comment threadsrc/libraries/System.Runtime/tests/System/CharTests.GenericMath.cs Outdated

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 0420af8Compare July 9, 2021 23:38

tannergooding merged commit bb39822 into dotnet:mainJul 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

-[Fact]
-publicstaticvoidTryCreateFromCharTest()
-{
-byteresult;
-Assert.True(NumberHelper<byte>.TryCreate<char>((char)0x0000,outresult));
-Assert.Equal((byte)0x00,result);
-Assert.True(NumberHelper<byte>.TryCreate<char>((char)0x0001,outresult));
-Assert.Equal((byte)0x01,result);
-Assert.False(NumberHelper<byte>.TryCreate<char>((char)0x7FFF,outresult));
-Assert.Equal((byte)0x00,result);
-Assert.False(NumberHelper<byte>.TryCreate<char>((char)0x8000,outresult));
-Assert.Equal((byte)0x00,result);
-Assert.False(NumberHelper<byte>.TryCreate<char>((char)0xFFFF,outresult));
-Assert.Equal((byte)0x00,result);
-}
+[Theory]
+[InlineData(0x0000,0x00)]
+[InlineData(0x0001,0x01)]
+[InlineData(0x7FFF,0x00)]
+[InlineData(0x8000,0x00)]
+[InlineData(0xFFFF,0x00)]
+publicstaticvoidTryCreateFromCharTest(intinput,intexpected)
+{
+Assert.True(NumberHelper<byte>.TryCreate<char>((char)input,outbyteresult));
+Assert.Equal((byte)expected,result);
+}

Uh oh!

Conversation

tannergooding commented Jul 8, 2021

Uh oh!

ghost commented Jul 8, 2021

Uh oh!

ghost commented Jul 8, 2021

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

lambdageek commented Jul 9, 2021

Uh oh!

BrzVlad commented Jul 9, 2021

Uh oh!

tannergooding commented Jul 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgovind left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants