quot overflow issues around Long/MIN_VALUE for BigInt

Description

In Clojure 1.5.1, see the following undesirable behavior regarding incorrect quot results for BigInts:

Similar issue to CLJ-1222. The root cause is that Java division of longs gives a numerically incorrect answer of Long.MIN_VALUE for (Long.MIN_VALUE / -1), because the numerically correct answer does not fit in a long. I believe this is the only pair of arguments for long division that gives a numerically incorrect answer, because division with a denominator having an absolute value of 2 or more gives a result closer to 0 than the numerator, and everything works fine for a denominator of 1 or -1, except this one case.

Related issues: CLJ-1222 for multiply, CLJ-1253 for / on longs, CLJ-1254 for quot on longs

Patch: clj-1225-2.txt
Screened by: Alex Miller

Environment

None

Activity

Show:
Andy Fingerhut
June 26, 2013, 3:03 AM

Patch clj-1225-fix-division-overflow-patch-v1.txt dated Jun 25 2013 may be one good way to address this issue. It modifies quot and / to return the numerically correct (BigInt) answer when given args Long/MIN_VALUE and -1.

It also removes the quotient intrinsic that does a JVM LDIV operation on longs for quot, since that operation is one of those that gives the incorrect result. I have not done any performance testing with this patch yet, but I have verified that it does not introduce any new reflection warnings when compiling Clojure itself.

Andy Fingerhut
June 26, 2013, 3:13 AM

Another possible approach would be to create unchecked-quotient and quot', which together with quot would correspond to the existing unchecked-multiply, *' and *. That is a more significant change. One potential concern it addresses that patch clj-1225-fix-division-overflow-patch-v1.txt does not is that patch leaves a Clojure developer with no way to do a primitive Java long division except by writing Java code.

Rich Hickey
September 6, 2013, 12:39 AM

this is two separate issues, one with longs and one with bigints. long problem should throw

Andy Fingerhut
September 6, 2013, 4:37 PM

Updating description for BigInt issue only. Will create separate ticket for incorrect behavior of / and quot on long type args Long/MIN_VALUE and -1.

Andy Fingerhut
September 6, 2013, 4:41 PM

Patch clj-1225-2.txt fixes this issue with quot on BigInts, with tests for quot and / on these values. / on BigInt worked fine before, but added the tests in case someone decides to change the implementation and forgets this corner case.

Completed

Assignee

Unassigned

Reporter

Andy Fingerhut

Labels

Approval

Ok

Patch

Code and Test

Fix versions

Affects versions

Priority

Minor