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.

Sign up for GitHub

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

Jump to bottom

Add AllBitsSet DIM #72961

Merged
merged 6 commits into from
Aug 12, 2022
Merged

Add AllBitsSet DIM #72961

merged 6 commits into from
Aug 12, 2022

Conversation

dakersnar
Copy link
Contributor

@dakersnar dakersnar commented Jul 27, 2022 •
edited
Loading

Fixes #69676

Continuation of effort here: #71356

I wanted to get this PR up early so it can be merged as soon as dotnet/linker#2865 is fixed.

Is there an easy way to unit test DIMs? I can add unit tests if that would be helpful/possible.

@dakersnar dakersnar requested a review from tannergooding July 27, 2022 20:33
@dotnet-issue-labeler
Copy link

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 ghost assigned dakersnar Jul 27, 2022
@ghost
Copy link

ghost commented Jul 27, 2022

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

I wanted to get this PR up early so it can be merged as soon as dotnet/linker#2865 is fixed.

Is there an easy way to unit test DIMs? I can add unit tests if that would be helpful/possible.

Author: dakersnar
Assignees: dakersnar
Labels:

area-System.Numerics, new-api-needs-documentation
Milestone: -

@dakersnar dakersnar added blocked Issue/PR is blocked on something - see comments NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) blocked Issue/PR is blocked on something - see comments labels Jul 27, 2022
@dakersnar
Copy link
Contributor Author

Seems like the bug was actually fixed, so this should be ready to merge

This was referenced Aug 5, 2022
Closed
Closed
tannergooding
tannergooding reviewed Aug 12, 2022
View reviewed changes
src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj Outdated
@@ -111,6 +112,7 @@
<Compile Include="System\NullableMetadataTests.cs" />
<Compile Include="System\NullableTests.cs" />
<Compile Include="System\NullReferenceExceptionTests.cs" />
<Compile Include="System\Numerics\IBinaryNumberTests.cs" />
Copy link
Member

Choose a reason for hiding this comment

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

nit: The other generic math files are under a special item group down on L262

dakersnar and others added 2 commits August 11, 2022 22:17
@dakersnar @tannergooding
Apply suggestions from code review
f3995e7 
Co-authored-by: Tanner Gooding <tagoo@outlook.com>
@dakersnar
Apply suggestions from code review
10fbf9b
dakersnar
dakersnar commented Aug 12, 2022
View reviewed changes
src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj
Comment on lines 263 to 268
<Compile Include="$(CommonTestPath)System\GenericMathHelpers.cs" Link="Common\System\GenericMathHelpers.cs" />
<Compile Include="System\Numerics\GenericMathDimHelpers.cs" />
<Compile Include="System\Numerics\IBinaryNumberTests.cs" />
<Compile Include="System\ByteTests.GenericMath.cs" />
<Compile Include="System\CharTests.GenericMath.cs" />
<Compile Include="System\DecimalTests.GenericMath.cs" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tannergooding does this placement look correct now?

Copy link
Member

Choose a reason for hiding this comment

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

LGTM.

@dakersnar dakersnar merged commit 5eb776d into dotnet:main Aug 12, 2022
@dakersnar dakersnar deleted the feature-69676-2 branch August 12, 2022 22:19
@ghost ghost locked as resolved and limited conversation to collaborators Sep 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tannergooding tannergooding tannergooding approved these changes

@stephentoub stephentoub stephentoub left review comments

Assignees

@dakersnar dakersnar

Labels
area-System.Numerics new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Add AllBitsSet to the IBinaryNumber Interface
3 participants
@dakersnar @stephentoub @tannergooding