sahvx655-wq commented on code in PR #387:
URL: https://github.com/apache/commons-validator/pull/387#discussion_r3367615655
##########
src/main/java/org/apache/commons/validator/GenericTypeValidator.java:
##########
@@ -405,10 +405,13 @@ public static Long formatLong(final String value, final
Locale locale) {
final Number num = formatter.parse(value, pos);
// If there was no error and we used the whole string
+ // NumberFormat returns a Long only when the value fits in a long;
+ // out-of-range input is returned as a Double, so a doubleValue()
+ // range check cannot be used here (Long.MAX_VALUE is not exactly
+ // representable as a double and would let 2^63 through).
Review Comment:
You're right on one half and it's worth splitting them apart. The two pos
conditions do different jobs. pos.getErrorIndex() == -1 only matters when the
parse fails outright, but in that case NumberFormat.parse returns null, and num
instanceof Long is already false for null, so that test was dead weight once
the range check became an instanceof. I've dropped it.
The pos.getIndex() == value.length() one isn't superfluous though: with
setParseIntegerOnly(true) a value like "123x" parses to a Long 123 and stops at
the first non-digit, leaving getIndex() at 3 against a length of 4, so without
the whole-string check that trailing rubbish would be accepted. That path had
no coverage, which is why it read as redundant, so I've added an assertNull for
"123x" alongside the overflow cases. Patched tree accepts the bounds and
rejects MAX+1, MIN-1 and "123x".
--
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]
