Skip to content

Add DivRem with DivisionRounding (WIP + Review request) #104589

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

Closed

Conversation

heathbm
Copy link
Contributor

@heathbm heathbm commented Jul 9, 2024

Partial implementation of #93568 (requesting review of general approach before implementing other types since this is mostly copy & paste).

Done:

  • Added DivisionRounding to System
  • Added default generic IBinaryInteger DivRem implementation
  • Added DivRem implementation for Int32
  • Added tests for all types of DivisionRounding for Int32.DivRem

Todo:

  • Add DivRem implementations for remaining types
  • Add Divide and Remainder methods for all types
  • Add remaining tests

Thanks for the feedback!

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, 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.

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, 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-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 9, 2024
Copy link
Contributor

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

@heathbm
Copy link
Contributor Author

heathbm commented Jul 11, 2024

@tannergooding Would you be able to comment on the Math.DivRem and IBinaryInteger.DivRem approaches or recommend someone who could? Thanks.
Is this acceptable, or is something lower-level expected for this?

@jeffhandley
Copy link
Member

@heathbm I was finally able to take a look at this. I think it's on the right track. I merged main into your branch and fixed one error in the suppression file. Are you still up for implementing the rest of it?

Thanks for your patience and your efforts on this.

Copy link
Contributor

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

Copy link
Contributor

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Numerics community-contribution Indicates that the PR has been added by a community member needs-author-action An issue or pull request that requires more info or actions from the author. new-api-needs-documentation no-recent-activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants