Clojurians Log v2

@gfredericks how much does test.check depend on goog.math.Long? I'm probably going to drop that dep since it's in fact covered by goog.math.Integer

I'm no expert, but from perusing the code, I'd say "a lot"

primarily in the random number generator

@dnolen didn't you add Long support to core when working on transit? I thought that motivated the addition? https://github.com/cognitect/transit-js/blob/f49c7bae88bfbb4b0c36966d8a987341e44a3359/src/com/cognitect/transit/types.js#L18

@alexmiller it's marked as legacy by Closure now and you can accomplish the same thing with Integer

it s also a massive pain because it's one of the few namespaces we use that needs AOT transformation - so it's really hampering REPL maintenance

@thheller hrm that might be true, but I recall some back and forth with test.check

Gary is not actually working on test.check anymore so someone would need to step up to do that work

@alexmiller https://github.com/clojure/test.check/blob/master/src/main/clojure/clojure/test/check/random/longs.cljs?

anything else really?

the random stuff is based on that too and I know a lot of care was taken to have the outputs match in clj and cljs

Gary did a talk about it https://www.youtube.com/watch?v=u0t-6lUvXHo

@alexmiller I guess what I mean is I don't see how goog.math.Long as an implementation detail for test.check here (over Integer) is meaningful

ClojureScript doesn't support it as an arithmetic value

nor Integer

the only things that support it are the predicates and that was for test.check by way of spec.alpha

I don't know anything about several pieces of this so I am not trying to debate you about it :)

ok 🙂

@alexmiller I can probably handle the long changes - assessing this change first for REPLs

(assuming we drop them, I'm taking the time to assess using Closure to compile the problem away - and we can punt that change down the road)

The Integer class can do 64 bit bit twiddling?

Even if so, I'd wonder if performance would get worse

shift right and left - it's really the same thing just an array of integers instead of two 32 bit integers

I'm somewhat skeptical that it will make a big difference on performance

given Long is deprecated and hard to use and Integer is not

I think Google has decided the more meaningful lib

That's the only issue I can think of

k, like I said, I'm trying to avoid dropping it

if this works then we can make a more gradual todo to drop it

Like alex said, the current rng impl gives identical results cross platform. That's not a crucial feature though, I just thought it was cool.

That's only relevant if you consider changing the algorithm to recover perf somehow

is that checked automatically during testing?

or is it more manual?

Hmm, it might be, I'll check

thanks!

The tests will fail if the cljs algorithm changes unilaterally

ah k, cool

The weirdest part of the current code is that the multiplication algorithm was inlined so that some unnecessary checks could be removed

(I.e., pasted/translated from the goog lib)