Compare exact values in DoubleValidator and FloatValidator range checks#410
Compare exact values in DoubleValidator and FloatValidator range checks#410sahvx655-wq wants to merge 1 commit into
Conversation
garydgregory
changed the title
garydgregory
left a comment
garydgregory
left a comment
There was a problem hiding this comment.
Hello @sahvx655-wq
Please rebase on git master and see my comments.
|
|
||
| private static final FloatValidator VALIDATOR = new FloatValidator(); | ||
|
|
||
| private static boolean isFinite(final Number value) { |
There was a problem hiding this comment.
Rebase on git master and reuse AbstractNumberValidator.isFinite(Number).
|
|
||
| private static final DoubleValidator VALIDATOR = new DoubleValidator(); | ||
|
|
||
| private static boolean isFinite(final Number value) { |
There was a problem hiding this comment.
Rebase on git master and reuse AbstractNumberValidator.isFinite(Number).
There was a problem hiding this comment.
Done. Rebased on master and dropped the local helper in both DoubleValidator and FloatValidator so they use the inherited isFinite(Number).
| 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 |
There was a problem hiding this comment.
Why is this not just BigDecimal.valueOf(maxExactInt) instead of new BigDecimal(BigInteger.valueOf(maxExactInt)) but since this is Double validator instead of a "Big" validator is might be OK...
There was a problem hiding this comment.
Simplified to BigDecimal.valueOf(maxExactInt) in the Double test. Same value, less noise.
| */ | ||
| @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(); |
There was a problem hiding this comment.
Reuse the superclass' compareTo(Number, Number).
There was a problem hiding this comment.
Switched over. Both minValue and maxValue in both validators now call the superclass compareTo(value, bound) instead of building the BigDecimal by hand.
| */ | ||
| @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(); |
There was a problem hiding this comment.
Reuse the superclass' compareTo(Number, Number).
| */ | ||
| @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(); |
There was a problem hiding this comment.
It looks like BigDecimal.valueOf(value.doubleValue()) looses precision.
There was a problem hiding this comment.
Right, that was the wrong basis. Going through compareTo(value, max) runs the value through toBigDecimal, which for a Float uses Float.toString, so the float is compared at its own precision (9.007199E15) rather than the extra digits doubleValue() invents when it widens the float. I adjusted the FloatValidatorTest expectations to that value.
sahvx655-wq
force-pushed
the
double-float-exact-bound
branch
from
6c209e2 to
abd6bba
Compare
|
Rebased on master and reused the pulled-up helpers throughout: both validators drop the local isFinite copy for the inherited isFinite(Number), and minValue/maxValue now defer to the superclass compareTo(Number, Number) rather than assembling the BigDecimal locally. The test bound is BigDecimal.valueOf(...) now too. On the float precision point: you were right that BigDecimal.valueOf(value.doubleValue()) was wrong. compareTo runs the value through toBigDecimal, and for a Float that uses Float.toString, so the comparison sits at the float's own decimal value (9.007199E15) instead of the trailing digits doubleValue() fabricates when it widens the float. That moved the FloatValidatorTest expectations onto 9.007199E15, which is the honest bound for a float at that magnitude. mvn clean verify is green. |
2 participants
The Number-typed range overloads on
DoubleValidatorandFloatValidatorare inherited fromAbstractNumberValidator, which testsvalue.doubleValue()againstbound.doubleValue(). While reviewing the exact-comparison work already done on theBigIntegerandBigDecimalvalidators I noticed the two floating-point validators still take that narrowing path, so aBigDecimalorBigIntegerbound carrying more significant digits than adoublecan hold is rounded onto the value before the test.minValue(2^53, BigInteger 2^53 + 1)returnstrueandmaxValue(2^53, 2^53 - 0.5)returnstrue, both wrong: a value that sits outside the bound is reported as in range, and it then slips through anyisInRangeguard built on these methods.The two overrides keep the bound as a
BigDecimaland compare the value'sdoubleagainst it, so the bound's precision survives; this follows thecompareTocomparisons already used inBigIntegerValidatorandBigDecimalValidator. A non-finite operand keeps the olddoubleValue()comparison so the documented infinity and NaN behaviour is unchanged. Placing the check in the callee meansisInRange(Number, Number, Number)picks it up through virtual dispatch rather than each caller needing its own guard.mvn; that'smvnon the command line by itself.