quot and rem handle doubles badly

Description

quot and rem in the doubles case (where any one of the arguments is a floating point) gives strange results for non-finite arguments:

quot and rem also do divide-by-zero checks for doubles, which is inconsistent with how doubles act for division:

Attached patch does not address this because I'm not sure if this is intended behavior. There were no tests asserting any of the behavior mentioned above.

Fundamentally the reason for this behavior is that the implementation for quot and rem sometimes (when result if division larger than a long) take a double, coerce it to BigDecimal, then BigInteger, then back to a double. The coersion means it can't handle nonfinite intermediate values. All of this is completely unnecessary, and I think is just leftover detritus from when these methods used to return a boxed integer type (long or BigInteger). That changed at this commit to return primitive doubles but the method body was not refactored extensively enough.

The method bodies should instead be simply:

Which is what the attached patch does. (And I'm not even sure the d==0 check is appropriate.)

Even if exploding on non-finite results is a desirable property of quot and rem, there is no need for the BigDecimal+BigInteger coersion. I can prepare a patch that preserves existing behavior but is more efficient.

More discussion at Clojure dev.

Environment

None

Activity

Show:
Andy Fingerhut
July 4, 2015, 6:12 AM

Francis, your patch clj-1680_no_div0.patch dated 2015-Mar-24 uses the method isFinite(), which appears to have been added in Java 1.8, and does not exist in earlier versions. I would guess that while the next release of Clojure may drop support for Java 1.6, it is less likely it would also drop support for Java 1.7 at the same time. It might be nice if your patch could use something like !(isInfinite() || isNaN()) instead, which I believe is equivalent, and both of those methods exist in earlier Java versions.

Francis Avila
July 27, 2015, 5:22 AM

Updated patch with a java 1.7-compatible version, also rebased against master.

No tests fail except this one, which I don't think is related to this patch:

Michael Blume
July 27, 2015, 7:34 PM

Francis, I tried downloading your patch and I didn't see any test failures at all. Do you see the same failure if you check out the master branch from the Clojure repo? Do you still see it if you mvn clean first? If so, it might be worth opening a ticket for it and seeing if anyone else can reproduce it.

Andy Fingerhut
July 27, 2015, 7:41 PM

Yes, and if you do see a failure with unmodified Clojure for 'mvn clean test', or './antsetup.sh ; ant clean; ant', please let us know the OS and JVM you are using. I haven't seen that on the OS/JVM combos I have tried.

Francis Avila
July 27, 2015, 8:51 PM

Nevermind, failing test went away after a clean. All tests pass.

Assignee

Unassigned

Reporter

Francis Avila

Labels

Approval

Triaged

Patch

Code and Test

Affects versions

Priority

Minor