garydgregory commented on code in PR #410:
URL: https://github.com/apache/commons-validator/pull/410#discussion_r3466970639
##########
src/main/java/org/apache/commons/validator/routines/DoubleValidator.java:
##########
@@ -68,6 +69,16 @@ public class DoubleValidator extends AbstractNumberValidator
{
private static final DoubleValidator VALIDATOR = new DoubleValidator();
+ private static boolean isFinite(final Number value) {
Review Comment:
Rebase on git master and reuse AbstractNumberValidator.isFinite(Number).
##########
src/test/java/org/apache/commons/validator/routines/DoubleValidatorTest.java:
##########
@@ -132,6 +134,26 @@ void testDoubleRangeMinMaxNaN() {
assertFalse(validator.maxValue(Double.NaN, 20), "maxValue() NaN");
}
+ /**
+ * Test the {@link Number} range checks against a bound that carries more
precision than a {@code double}.
+ * 2^53 is the largest integer with an exact {@code double}
representation, so 2^53 + 1 cannot be narrowed
+ * onto the value: a value of 2^53 is below a minimum of 2^53 + 1 and
above a maximum of 2^53 - 0.5.
+ */
+ @Test
+ void testDoubleNumberRangeExactBound() {
+ final DoubleValidator validator = (DoubleValidator) strictValidator;
+ final long maxExactInt = 1L << 53; // 2^53
+ final Double value = Double.valueOf(maxExactInt);
+ final BigInteger above =
BigInteger.valueOf(maxExactInt).add(BigInteger.ONE); // 2^53 + 1
+ final BigInteger below =
BigInteger.valueOf(maxExactInt).subtract(BigInteger.ONE); // 2^53 - 1
+ final BigDecimal justBelow = new
BigDecimal(BigInteger.valueOf(maxExactInt)).subtract(BigDecimal.valueOf(0.5));
// 2^53 - 0.5
Review Comment:
Why is this not just `BigDecimal.valueOf(maxExactInt)` instead of `new
BigDecimal(BigInteger.valueOf(maxExactInt))`?
##########
src/main/java/org/apache/commons/validator/routines/DoubleValidator.java:
##########
@@ -183,6 +194,42 @@ public boolean minValue(final Double value, final double
min) {
return minValue(value.doubleValue(), min);
}
+ /**
+ * Tests if the value is less than or equal to a maximum, comparing the
exact values.
+ *
+ * <p>
+ * This overrides the {@link Number} overload inherited from the
superclass, which narrows the bound to a {@code double} before comparing and so
loses
+ * precision for a {@code BigDecimal} or {@code BigInteger} bound that
carries more significant digits than a {@code double} can hold. A non-finite
+ * {@link Double} or {@link Float} operand keeps the {@code doubleValue()}
comparison so the documented infinity behaviour is unchanged.
+ * </p>
+ *
+ * @param value The value validation is being performed on.
+ * @param max The maximum value.
+ * @return {@code true} if the value is less than or equal to the maximum.
+ */
+ @Override
+ public boolean maxValue(final Number value, final Number max) {
+ return isFinite(value) && isFinite(max) ?
BigDecimal.valueOf(value.doubleValue()).compareTo(toBigDecimal(max)) <= 0 :
value.doubleValue() <= max.doubleValue();
Review Comment:
Reuse the superclass' compareTo(Number, Number).
##########
src/main/java/org/apache/commons/validator/routines/FloatValidator.java:
##########
@@ -183,6 +194,42 @@ public boolean minValue(final Float value, final float
min) {
return minValue(value.floatValue(), min);
}
+ /**
+ * Tests if the value is less than or equal to a maximum, comparing the
exact values.
+ *
+ * <p>
+ * This overrides the {@link Number} overload inherited from the
superclass, which narrows the bound to a {@code double} before comparing and so
loses
+ * precision for a {@code BigDecimal} or {@code BigInteger} bound that
carries more significant digits than a {@code double} can hold. A non-finite
+ * {@link Double} or {@link Float} operand keeps the {@code doubleValue()}
comparison so the documented infinity behaviour is unchanged.
+ * </p>
+ *
+ * @param value The value validation is being performed on.
+ * @param max The maximum value.
+ * @return {@code true} if the value is less than or equal to the maximum.
+ */
+ @Override
+ public boolean maxValue(final Number value, final Number max) {
+ return isFinite(value) && isFinite(max) ?
BigDecimal.valueOf(value.doubleValue()).compareTo(toBigDecimal(max)) <= 0 :
value.doubleValue() <= max.doubleValue();
Review Comment:
It looks like `BigDecimal.valueOf(value.doubleValue())` looses precision.
##########
src/main/java/org/apache/commons/validator/routines/FloatValidator.java:
##########
@@ -68,6 +69,16 @@ public class FloatValidator extends AbstractNumberValidator {
private static final FloatValidator VALIDATOR = new FloatValidator();
+ private static boolean isFinite(final Number value) {
Review Comment:
Rebase on git master and reuse AbstractNumberValidator.isFinite(Number).
##########
src/main/java/org/apache/commons/validator/routines/DoubleValidator.java:
##########
@@ -183,6 +194,42 @@ public boolean minValue(final Double value, final double
min) {
return minValue(value.doubleValue(), min);
}
+ /**
+ * Tests if the value is less than or equal to a maximum, comparing the
exact values.
+ *
+ * <p>
+ * This overrides the {@link Number} overload inherited from the
superclass, which narrows the bound to a {@code double} before comparing and so
loses
+ * precision for a {@code BigDecimal} or {@code BigInteger} bound that
carries more significant digits than a {@code double} can hold. A non-finite
+ * {@link Double} or {@link Float} operand keeps the {@code doubleValue()}
comparison so the documented infinity behaviour is unchanged.
+ * </p>
+ *
+ * @param value The value validation is being performed on.
+ * @param max The maximum value.
+ * @return {@code true} if the value is less than or equal to the maximum.
+ */
+ @Override
+ public boolean maxValue(final Number value, final Number max) {
+ return isFinite(value) && isFinite(max) ?
BigDecimal.valueOf(value.doubleValue()).compareTo(toBigDecimal(max)) <= 0 :
value.doubleValue() <= max.doubleValue();
+ }
+
+ /**
+ * Tests if the value is greater than or equal to a minimum, comparing the
exact values.
+ *
+ * <p>
+ * This overrides the {@link Number} overload inherited from the
superclass, which narrows the bound to a {@code double} before comparing and so
loses
+ * precision for a {@code BigDecimal} or {@code BigInteger} bound that
carries more significant digits than a {@code double} can hold. A non-finite
+ * {@link Double} or {@link Float} operand keeps the {@code doubleValue()}
comparison so the documented infinity behaviour is unchanged.
+ * </p>
+ *
+ * @param value The value validation is being performed on.
+ * @param min The minimum value.
+ * @return {@code true} if the value is greater than or equal to the
minimum.
+ */
+ @Override
+ public boolean minValue(final Number value, final Number min) {
+ return isFinite(value) && isFinite(min) ?
BigDecimal.valueOf(value.doubleValue()).compareTo(toBigDecimal(min)) >= 0 :
value.doubleValue() >= min.doubleValue();
Review Comment:
Reuse the superclass' compareTo(Number, Number).
--
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]
