aherbert commented on code in PR #225:
URL: https://github.com/apache/commons-numbers/pull/225#discussion_r4472682138
##########
commons-numbers-core/src/main/java/org/apache/commons/numbers/core/DD.java:
##########
@@ -753,11 +753,16 @@ public long longValue() {
// with respect to the sign of the high part.
final long b = (long) (a < 0 ? Math.ceil(xx) : Math.floor(xx));
- final long sum = a1 + b + a2;
- // Avoid overflow. If the sum has changed sign then an overflow
occurred.
- // This happens when high == 2^63 and the low part is additional
magnitude.
- // The xor operation creates a negative if the signs are different.
- if ((sum ^ a) >= 0) {
Review Comment:
The original overflow check was incorrect. Your new test passes using this
modified check:
```java
final long sum = a1 + b + a2;
// Avoid overflow.
// If the sum of equal signed parts has changed sign then an
overflow occurred.
// This happens when high == 2^63 and the low part is additional
magnitude.
// The xor operation creates a negative if the signs are
different.
// Check the signs using a1 + b as the sign of a2 matches a1 or
it was zero.
if (((sum ^ a1) & (sum ^ b)) >= 0) {
return sum;
}
```
This avoids use of Math.addExact and logic flow using exception handling.
##########
commons-numbers-core/src/test/java/org/apache/commons/numbers/core/DDTest.java:
##########
@@ -383,6 +383,31 @@ void testLongSum(long[] values) {
Assertions.assertEquals(expected, dd.longValue(), "long value");
}
+ @Test
+ void testLongValueTruncatesTowardsZeroWhenTheLowPartReversesTheSign() {
Review Comment:
This method should go above testLongSum so all the methods testing
toLong/toInt are together.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
