[CLJ-1222] Multiplication overflow issues around Long/MIN_VALUE Created: 21/Jun/13 Updated: 22/Nov/13 Resolved: 22/Nov/13
|Fix Version/s:||Release 1.6|
|Patch:||Code and Test|
Long story short (ha!), there are several related issues where overflow checking doesn't work correctly with Long/MIN_VALUE. The cause of this missed check is that in Java, these both pass:
This causes the following results in Clojure that do not match their siblings where the order of operands is switched:
Mailing list discussion here: https://groups.google.com/d/msg/clojure-dev/o1QGR1QK70M/A1KykR9OQqwJ
Approach: Modifies BigInt.multiply() and multiply/multiplyP in Numbers to take into account Long.MIN_VALUE.
|Comment by Andy Fingerhut [ 21/Jun/13 12:56 PM ]|
Is there some proof that Long/MIN_VALUE and -1 is the only pair of values that causes a problem with the way overflow-checking is currently done? It might be, but it would be nice to be sure if a proposed patch relies on that.
|Comment by Colin Jones [ 21/Jun/13 1:54 PM ]|
I don't have a proof, just a half-hour or so of digging for an alternate case (found none), plus taking a look at what Guava does in checkedMultiply: https://code.google.com/p/guava-libraries/source/browse/guava/src/com/google/common/math/LongMath.java#522
I didn't do the leading zeros optimization they're doing to avoid even the division in the overflow check we already have; that felt like a bigger change, and not one that impacts correctness.
|Comment by Andy Fingerhut [ 25/Jun/13 11:06 AM ]|
I have created
|Comment by Rich Hickey [ 22/Nov/13 7:28 AM ]|
user=> (*' Long/MIN_VALUE -1)
Is wrong - there should be no coercion, this is an overflowing operation.
|Comment by Colin Jones [ 22/Nov/13 8:14 AM ]|
Rich, can you clarify? My understanding is that *' is an auto-promoting operation. For instance, in 1.5.1, *' auto-promotes in the positive direction:
user=> (*' Long/MAX_VALUE Long/MAX_VALUE)
and in the negative:
user=> (*' Long/MIN_VALUE -2)
The only outlier is the one addressed in this ticket. Am I missing something? What should the result be here if not the one given by this patch?
|Comment by Rich Hickey [ 22/Nov/13 10:14 AM ]|
missed the ' earlier