Fork me on GitHub
#cljs-dev
<
2020-03-26
>
dnolen16:03:51

@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

Alex Miller (Clojure team)16:03:57

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

Alex Miller (Clojure team)16:03:09

primarily in the random number generator

thheller17:03:27

@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

dnolen17:03:13

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

dnolen17:03:58

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

dnolen17:03:58

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

Alex Miller (Clojure team)17:03:12

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

dnolen17:03:48

anything else really?

Alex Miller (Clojure team)17:03:00

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

dnolen18:03:22

@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

dnolen18:03:56

ClojureScript doesn't support it as an arithmetic value

dnolen18:03:22

nor Integer

dnolen18:03:02

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

Alex Miller (Clojure team)18:03:10

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

dnolen18:03:49

ok 🙂

dnolen18:03:00

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

dnolen18:03:21

(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)

gfredericks21:03:40

The Integer class can do 64 bit bit twiddling?

gfredericks21:03:47

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

dnolen21:03:17

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

dnolen21:03:32

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

dnolen21:03:48

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

dnolen21:03:55

I think Google has decided the more meaningful lib

gfredericks21:03:06

That's the only issue I can think of

dnolen21:03:43

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

dnolen21:03:06

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

gfredericks21:03:11

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.

gfredericks21:03:27

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

dnolen21:03:34

is that checked automatically during testing?

dnolen21:03:41

or is it more manual?

gfredericks21:03:51

Hmm, it might be, I'll check

gfredericks21:03:10

The tests will fail if the cljs algorithm changes unilaterally

dnolen21:03:29

ah k, cool

gfredericks21:03:23

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

gfredericks22:03:16

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