Welcome! Please see the About page for a little more info on how this works.

0 votes
in ClojureScript by

The implementation of the quot and rem functions are needlessly complicated. Currently they are:

(defn quot [n d] (fix (/ (- n (js-mod n d)) d))) (defn rem [n d] (- n (* d (quot n d))))

However all numbers in js are doubles already, so all this is unnecessary:

(defn quot [n d] (fix (/ n d))) (defn rem [n d] (js-mod n d)))

Notice that "rem" is simply js-mod, and I'm not sure why no one noticed this before. I keep js-mod for now since a lot of code uses it, and if cljs ever grows a number tower the distinction may be important.

Patch attached, which also:

  • Creates a macro version of quot and rem.
  • Updates documentation for quot, rem, js-mod and mod for clarity.
  • Implement fix (private function to round to zero) with ES6 Math.trunc() if available.

Existing quot and rem tests pass, although there could be some better tests of edge cases (negative decimal num or div, NaN and +-Infinity args).

9 Answers

0 votes
by

Comment made by: favila

Better tests found rounding errors in my updated {{rem}}, which should stay as-is. (Not simply js-mod after all! Seems to round args first? Not obvious from the spec.) Changed quot however is correct and introduces less error than the current one. Will update patch and tests when I get a chance.

0 votes
by

Comment made by: favila

Working patch with tests attached. Tests expanded to cover floating-point cases. {{rem}} is now fundamentally the same as master (was more accurate than js-mod!!), but returns results consistent with js-mod for non-finite args or zero divisor.

0 votes
by

Comment made by: mfikes

cljs-1164.patch no longer applies on master

0 votes
by

Comment made by: arichiardi

Patch now applies. I only tested with Nashorn:

`

V8_HOME not set, skipping V8 tests
SPIDERMONKEY_HOME not set, skipping SpiderMonkey tests
JSC_HOME not set, skipping JavaScriptCore tests
Testing with Nashorn

...

Ran 185 tests containing 17195 assertions.
0 failures, 0 errors.
Tested with 1 out of 4 possible js targets
`

0 votes
by

Comment made by: arichiardi

Patch cleaned up

0 votes
by

Comment made by: mfikes

Successfully ran Andrea's update to Francis's patch through V8, SpiderMonkey, JavaScriptCore, and Nashorn unit tests.

I also manually ran some of the unit tests in bootstrapped ClojureScript built with the patch.

LGTM.

0 votes
by

Comment made by: mfikes

Since this is a low-level numerics update, also ran the unit tests through ChackraCore (successfully).

0 votes
by

Comment made by: mfikes

CLJS-1164-1.patch no longer applies to master

0 votes
by
Reference: https://clojure.atlassian.net/browse/CLJS-1164 (reported by favila)
...