Skip to content

Conversation

@AlexRadch
Copy link
Contributor

Close #93568

@ghost
Copy link

ghost commented Aug 22, 2024

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.

@ghost
Copy link

ghost commented Aug 22, 2024

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 Aug 22, 2024
@huoyaoyuan
Copy link
Member

This is also duplicated with #104589

@AlexRadch
Copy link
Contributor Author

AlexRadch commented Aug 22, 2024

This is also duplicated with #104589

I overlooked this PR. My PR has many tests that you can use.

Corrected the file path for `DivisionRounding.cs` from a lowercase 's' in "system" to an uppercase 'S' in "System" to ensure consistency with the project structure.
@tannergooding tannergooding requested a review from Copilot October 24, 2025 16:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements support for alternative division rounding modes by introducing a new DivisionRounding enum and extending the IBinaryInteger<TSelf> interface with new overloads for Divide, DivRem, and Remainder methods that accept a DivisionRounding parameter. The implementation provides five rounding strategies: Truncate, Floor, Ceiling, AwayFromZero, and Euclidean.

Key Changes:

  • Added DivisionRounding enum with five rounding modes
  • Extended IBinaryInteger<TSelf> interface with rounding mode overloads for division operations
  • Added comprehensive test coverage for all integer types across all rounding modes

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
DivisionRounding.cs Defines the new enum with five division rounding modes
IBinaryInteger.cs Implements default logic for division operations with rounding modes
System.Runtime.cs Adds API surface for the new enum and interface methods
System.Private.CoreLib.Shared.projitems Registers the new DivisionRounding.cs file in the build
GenericMathHelpers.cs Adds test helper methods to compute expected values for different rounding modes
*Tests.GenericMath.cs (multiple) Expands existing DivRem tests and adds new tests for mode-based division operations

// Graph for ceiling division with negative divisor https://www.wolframalpha.com/input?i=Plot%5B%7BCeiling%5B-n%5D%2C+n+%2B+Ceiling%5B-n%5D%7D%2C+%7Bn%2C+-3%2C+3%7D%5D

/// <summary>
/// AwayFromZero division (rounding away zero — round the division result away from zero to the nearest integer.
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

Missing closing parenthesis in the documentation comment. The opening parenthesis after 'zero' should be closed before the em dash.

Suggested change
/// AwayFromZero division (rounding away zero — round the division result away from zero to the nearest integer.
/// AwayFromZero division (rounding away zero) — round the division result away from zero to the nearest integer.

Copilot uses AI. Check for mistakes.
}
default:
ThrowHelper.ThrowArgumentException_InvalidEnumValue(mode);
break;
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

The ThrowArgumentException_InvalidEnumValue call is not wrapped in a throw statement. While ThrowHelper methods typically throw exceptions internally, this break statement is unreachable and should be removed for clarity, or the method call should explicitly throw.

Suggested change
break;

Copilot uses AI. Check for mistakes.
break;
}
default:
ThrowHelper.ThrowArgumentException_InvalidEnumValue(mode);
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

The ThrowArgumentException_InvalidEnumValue call is not wrapped in a throw statement. While ThrowHelper methods typically throw exceptions internally, this break statement is unreachable and should be removed for clarity, or the method call should explicitly throw.

Copilot uses AI. Check for mistakes.
}
default:
ThrowHelper.ThrowArgumentException_InvalidEnumValue(mode);
break;
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

The ThrowArgumentException_InvalidEnumValue call is not wrapped in a throw statement. While ThrowHelper methods typically throw exceptions internally, this break statement is unreachable and should be removed for clarity, or the method call should explicitly throw.

Suggested change
break;

Copilot uses AI. Check for mistakes.
Comment on lines +164 to +166
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivRemExpected((nint)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivRemExpected((nint)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivRemExpected((nint)0xFFFFFFFFFFFFFFFF, 0, mode));
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

These assertions test DivRemExpected instead of DivRem. The test should verify that DivRem throws the exception, not the helper method. Change to BinaryIntegerHelper<nint>.DivRem(...).

Suggested change
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivRemExpected((nint)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivRemExpected((nint)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivRemExpected((nint)0xFFFFFFFFFFFFFFFF, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivRem((nint)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivRem((nint)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivRem((nint)0xFFFFFFFFFFFFFFFF, 0, mode));

Copilot uses AI. Check for mistakes.
Comment on lines +228 to +230
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivideExpected((nint)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivideExpected((nint)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivideExpected((nint)0xFFFFFFFFFFFFFFFF, 0, mode));
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

These assertions test DivideExpected instead of Divide. The test should verify that Divide throws the exception, not the helper method. Change to BinaryIntegerHelper<nint>.Divide(...).

Suggested change
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivideExpected((nint)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivideExpected((nint)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.DivideExpected((nint)0xFFFFFFFFFFFFFFFF, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.Divide((nint)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.Divide((nint)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.Divide((nint)0xFFFFFFFFFFFFFFFF, 0, mode));

Copilot uses AI. Check for mistakes.
Comment on lines +291 to +293
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.RemainderExpected((nint)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.RemainderExpected((nint)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.RemainderExpected((nint)0xFFFFFFFFFFFFFFFF, 0, mode));
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

These assertions test RemainderExpected instead of Remainder. The test should verify that Remainder throws the exception, not the helper method. Change to BinaryIntegerHelper<nint>.Remainder(...).

Suggested change
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.RemainderExpected((nint)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.RemainderExpected((nint)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.RemainderExpected((nint)0xFFFFFFFFFFFFFFFF, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.Remainder((nint)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.Remainder((nint)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<nint>.Remainder((nint)0xFFFFFFFFFFFFFFFF, 0, mode));

Copilot uses AI. Check for mistakes.
Comment on lines +110 to +112
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivRemExpected((long)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivRemExpected((long)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivRemExpected((long)0xFFFFFFFFFFFFFFFF, 0, mode));
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

These assertions test DivRemExpected instead of DivRem. The test should verify that DivRem throws the exception, not the helper method. Change to BinaryIntegerHelper<long>.DivRem(...).

Suggested change
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivRemExpected((long)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivRemExpected((long)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivRemExpected((long)0xFFFFFFFFFFFFFFFF, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivRem((long)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivRem((long)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivRem((long)0xFFFFFFFFFFFFFFFF, 0, mode));

Copilot uses AI. Check for mistakes.
Comment on lines +144 to +146
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivideExpected((long)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivideExpected((long)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivideExpected((long)0xFFFFFFFFFFFFFFFF, 0, mode));
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

These assertions test DivideExpected instead of Divide. The test should verify that Divide throws the exception, not the helper method. Change to BinaryIntegerHelper<long>.Divide(...).

Suggested change
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivideExpected((long)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivideExpected((long)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.DivideExpected((long)0xFFFFFFFFFFFFFFFF, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.Divide((long)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.Divide((long)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.Divide((long)0xFFFFFFFFFFFFFFFF, 0, mode));

Copilot uses AI. Check for mistakes.
Comment on lines +178 to +180
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.RemainderExpected((long)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.RemainderExpected((long)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.RemainderExpected((long)0xFFFFFFFFFFFFFFFF, 0, mode));
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

These assertions test RemainderExpected instead of Remainder. The test should verify that Remainder throws the exception, not the helper method. Change to BinaryIntegerHelper<long>.Remainder(...).

Suggested change
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.RemainderExpected((long)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.RemainderExpected((long)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.RemainderExpected((long)0xFFFFFFFFFFFFFFFF, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.Remainder((long)0x0000000000000000, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.Remainder((long)0x0000000000000001, 0, mode));
Assert.Throws<DivideByZeroException>(() => BinaryIntegerHelper<long>.Remainder((long)0xFFFFFFFFFFFFFFFF, 0, mode));

Copilot uses AI. Check for mistakes.
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 new-api-needs-documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide support for alternative rounding modes for division and remainder of division.

3 participants