Fork me on GitHub
#cljs-dev
<
2017-06-28
>
rauh12:06:16

Somehow this commit: https://github.com/clojure/clojurescript/commit/940b6c8ffd98754f3dfc0e7fbe0878235f62debd Made CLJS emit tons of with-meta call with ::analyzed true.

dnolen12:06:40

@rauh does it matter?

dnolen12:06:04

I’d be very surprised if it does

rauh12:06:23

I mean it's emitted into the JS code every time fexpr__ is there.

dnolen12:06:48

what is emitted?

dnolen12:06:02

you need to be more clear 🙂

dnolen12:06:15

“emitted” like during profiling

dnolen12:06:29

or are you talking about codegen?

dnolen12:06:35

if codegen show an example

rauh12:06:39

Codegen, yes! I'll give an example

dnolen12:06:33

oh I think I know what you mean, we don’t elide that meta so for literals it will drop meta

rauh12:06:00

var new_root = (function() {
      var fexpr__11505 = cljs.core.with_meta(((function(tcoll__$1) {
       /// SNIP
          })(tcoll__$1))
,new cljs.core.PersistentArrayMap(null, 1, [cljs.core.cst$kw$cljs$analyzer_SLASH_analyzed,true], null));
return (fexpr__11505.cljs$core$IFn$_invoke$arity$2 ? fexpr__11505.cljs$core$IFn$_invoke$arity$2(self__.shift,self__.root) : fexpr__11505(self__.shift,self__.root));
})();

dnolen12:06:50

@rauh that’s not useful 🙂

dnolen12:06:56

we need the ClojureScript source

dnolen12:06:13

I do understand what’s happening

dnolen12:06:37

(and probably already fixed it) but would like example case

rauh13:06:08

Seems to happen for: (#{> >=} test) and ((fn [...]) immediate-invoke) expressions.

rauh13:06:27

Not so for function invokes (lists).

rauh13:06:25

Does that help?

dnolen13:06:43

@rauh yep fixed in master

rauh13:06:12

@dnolen Cool. Thanks.

dnolen13:06:34

we’ll see what else shakes out this week and do a small fixes release

rauh13:06:49

Could you sneak in a - in the with-meta call on MapEntry if you get a chance?

dnolen13:06:25

seems very low priority 🙂

rauh13:06:43

Ok seems fair 🙂

rauh13:06:56

Is there a ticket yet for this:

(defn foo
  ([a & b] 0)
  ([a b & c] 1))

(defn foo
  ([a b c d e] 0)
  ([a b & c] 1))

rauh13:06:27

Both of these don't emit any warning/error. Just trying to avoid problems like we had with the variadic protocol usage.

dnolen14:06:41

I believe there is, look in JIRA

favila14:06:12

I wanted to do this before there were too many in-the-wild -find impls that assumed a contains? was already done

dnolen14:06:56

@favila looks OK I left my only comment in the ticket - revert the docstring change to the protocol and I’ll push it through

favila14:06:48

Would you accept the same docstring for find on -find itself? (not the protocol). I.e.

favila14:06:51

(defprotocol IFind
  "Protocol for implementing entry finding in collections."
  (-find [coll k] "Returns the map entry for key, or nil if key not present."))

dnolen14:06:57

this is for implementers

dnolen14:06:01

they will know when they mess it up

favila14:06:03

All other protocols in the area (ILookup, IAssociative, IMap, etc) do this. And the confusion for implementors was the point of the patch

dnolen14:06:44

enumerate all assumptions - they do no such thing

favila14:06:50

(defprotocol IMap
  "Protocol for adding mapping functionality to collections."
  #_(-assoc-ex [coll k v])
  (^clj -dissoc [coll k]
    "Returns a new collection of coll without the mapping for key k."))

favila14:06:03

(^clj -assoc [coll k v]
    "Returns a new collection of coll with a mapping from key k to
     value v added to it.")

dnolen14:06:17

my answer is no

dnolen14:06:25

just remove the docstring changes

favila15:06:35

So you think implementors will never be unclear whether they might to return nil? even though this has already happened in CLJS-2013?

dnolen15:06:36

there so many things that can go wrong when you’re build custom data structures

dnolen15:06:41

this just isn’t our problem

dnolen15:06:57

write tests, use generative tests, whatever

favila15:06:25

@dnolen Updated patch. But still disagree, and don't understand why almost all other collection protocols in core document their return values but this one cannot

dnolen15:06:48

@favila sorry I think we were probably talking past each other here

dnolen15:06:07

I do not want the containment check subtlely in the docstring

favila15:06:44

Did you not notice this counterproposal?

dnolen15:06:33

@favila yes I misread what you said

dnolen15:06:45

assuming you want to keep the same docstring

dnolen15:06:03

I apologize

dnolen15:06:21

I just did not want anything at all anywhere about the containment check stuff

favila15:06:49

would adding "Returns the map entry for key, or nil if key not present." to the -find method itself be acceptable?

favila15:06:54

That would satisfy me

favila15:06:14

Ok, thanks for slowing down, I appreaciate it

favila15:06:18

updating patch again

dnolen15:06:31

sorry for not reading what you said more closely

tmulvaney15:06:04

I'm not sure if the change needs to be made to PTMs IFind?

tmulvaney15:06:26

I think it is already returning nil or a Red/Black node isn't it?

favila15:06:49

@tmulvaney It doesn't. Those impls were find and working, I just made them use case instead

favila15:06:00

you coded those defensively

tmulvaney15:06:10

ah I meant this part in the patch:

IFind
   (-find [coll k]
-    (.entry-at coll k))
+    (when-some [found (.entry-at coll k)]
+      [(.-key found) (.-val found)]))

favila15:06:28

oh, let me double check

favila15:06:36

I think it was not ok

favila15:06:56

@tmulvaney nevermind, you may be right. Running the tests again

favila15:06:01

just noticed the warning

favila15:06:55

is that meant to be -assoc-n?

tmulvaney15:06:01

Not much, I just needed to implement IVector for the test

tmulvaney15:06:14

it might as well be an empty body in the method really as its not called

tmulvaney15:06:45

I seem to be really bad at noticing warnings from the compiler!

favila15:06:00

it's much clearer with test-self-parity

favila15:06:19

test has a lot of noise about compiling files and timing

favila15:06:04

anyway, you were right, RedNode and BlackNode are fine as-is, no need to pull key and val out of them

favila15:06:09

updating the patch

tmulvaney15:06:09

no worries. That patch taught me about the existence of when-some/if-some. My new favourite binding forms 😀

favila15:06:34

yeah I never use when/if-let anymore

favila15:06:43

Almost think they should be deprecated

favila15:06:09

many subtle bugs where false is an acceptable value

tmulvaney15:06:20

yeah i'm surprised i never knew about it until now. very handy indeed

favila15:06:05

@dnolen @tmulvaney https://dev.clojure.org/jira/browse/CLJS-2136 updated. should be ready now but review always appreciated

tmulvaney15:06:13

Does IFind on PersistentArrayMap have to be updated?

tmulvaney15:06:36

I don't think it the current method checks whether the map has the key or not

tmulvaney15:06:42

You've updated my map_entry_tests, cheers!

favila15:06:43

IFind
  (-find [coll k]
    (let [idx (array-map-index-of coll k)]
      (when-not (== idx -1)
        [(aget arr idx) (aget arr (inc idx))])))

favila15:06:56

that's new the PAM

tmulvaney15:06:05

ah yeah missed that

tmulvaney16:06:32

looks like find on vector works correctly in the patch with respect to non integer keys

tmulvaney16:06:40

get should probably be updated to do the same as (get [0 1 2 3] 1.42) returning 1 is weird

favila16:06:41

oh is that still there?

favila16:06:23

but I cant' find an issue for it. I was sure it had been mentioned before

favila16:06:46

in doing this I noticed that integer? is weird and slow

favila16:06:20

I'm not sure what it's supposed to do. could be 1) exactly what clj does for all input, 2) only safely-representable longs in js (53-bit or less), 3) what the docstring says, any number without decimal components.

favila16:06:31

but there's a test (is (= (integer? 1e308) false)) which makes 3 not work, but if you change it to do that, then other tests fail

favila16:06:00

that's why I use (== n (bit-or n 0)) in there

favila16:06:20

it used to be integer? until I dug in to it and it scared me away

dnolen16:06:02

there’s no sensible way to check for floating point - we do not care about this case

dnolen16:06:21

any validation of key must happen inside -find

dnolen16:06:25

and only fast validation is allowed

dnolen16:06:59

so if cannot validate quickly just leave it alone

favila16:06:09

@dnolen we're not really talking about the -find patch at this point anymore I think. @tmulvaney noticed that whatever I do in vector -find would probably be good in vector -lookup WRT float keys

dnolen16:06:35

like I said, don’t even think about it 🙂

dnolen16:06:39

don’t care about float keys

favila16:06:07

(== n (bit-or n 0)) is pretty fast

dnolen16:06:25

because it’s not right either

dnolen16:06:49

your clamping to 32bit

favila16:06:52

it's enough to verify 32 bit int

favila16:06:00

we have vectors longer than that?

dnolen16:06:01

but vector size doesn’t have that limit

bronsa16:06:53

FWIW clojure does have a 32bit limit

favila16:06:17

The bit arithmetic inside PV doesn't even work with sizes that big

dnolen16:06:25

anyways I’ll reiterate, I do not care about handling floating point

bronsa16:06:09

(neither does clojure in some cases, so I'm with dnolen here)

user=> (nth [1 2 3] 0.2)
1

favila16:06:08

(find [1 2 3] 0.2)
=> nil
(nth [1 2 3] 0.2)
=> 1

favila16:06:11

(that's Clojure)

favila16:06:27

so I appear to have inadvertently replicated clojure

bronsa16:06:33

yeah in clojure find/get care, nth doesn't

favila16:06:40

in cljs find cares, get doesn't, not sure about nth

dnolen16:06:10

caring and not caring - but this is about undefined behavior

dnolen16:06:56

@favila your patch looks OK, applied

favila16:06:55

@dnolen thanks. Even with that 32-bit clamping?

dnolen16:06:30

@favila it’s easy to remove after the fact, otherwise the patch was fine

dnolen16:06:58

I dropped the integer check bit in master

rauh17:06:43

@favila Subvec seems wrong, on?

(when (<= end (+ start n))
      (-find v (+ start n))

favila17:06:57

I relied on -find of v working properly. Do I need another bounds check to ensure I'm within the window subvec has?

favila17:06:33

bah there are no subvec find tests

dnolen17:06:40

yeah the order of check is wrong

dnolen17:06:52

and adding a test

favila17:06:55

@dnolen removing the bounds check is going to cause at least one test to fail: (is (= (find [1 2 3] 1.2) nil)) in collections-test

favila17:06:24

so if you are intent on this remember to remove it

favila17:06:40

sorry not bounds check, int check

favila17:06:00

Ah I see now @rauh, I needed (neg? n) to make sure no one escapes the start of the subvec window with a negative number

dnolen17:06:07

(< (+ start n) end) seems right to me

dnolen17:06:12

that’s what I’m about to commit

favila17:06:21

@dnolen I think if n is neg, you can look at items to the left of the subvec window

dnolen17:06:33

(and (pos? n) (< (+ start n) end))

dnolen17:06:26

actually need to check not neg?

rauh17:06:29

I think we should copy the existing code over:

IIndexed
  (-nth [coll n]
    (if (or (neg? n) (<= end (+ start n)))
      (vector-index-out-of-bounds n (- end start))
      (-nth v (+ start n))))

favila17:06:41

everywhere else it's expressed as when-not (or (neg? n) (<= end (+ start n)))

rauh17:06:10

Or I guess, nil instead of error, since that's what the -find docstring says

favila17:06:51

(-find [coll n]
    (when-not (or (neg? n) (<= end (+ start n)))
      (-find v (+ start n))))

favila17:06:04

but @dnolen 's suggestion should be equivalent

rauh17:06:13

Yup. Agreed

favila17:06:28

Actually @rauh, @dnolen version breaks for n = 0

favila17:06:42

0 is not pos?

dnolen17:06:08

(and (not (neg? n)) (< (+ start n) end))

rauh17:06:52

IMHO, copying the impl and 1:1 and even writing (if ... nil (-find ...)) might be easier for maintenance

favila17:06:57

isn't end inclusive?

favila17:06:04

if not then -lookup is wrong

dnolen17:06:09

not inclusive

dnolen17:06:25

(subvec [0 1 2] 0 2) -> [0 1]

favila17:06:41

I mean the internal end

favila17:06:03

@dnolen nevermind, I read the not in the wrong place

favila17:06:22

but I'm starting to agree with @rauh that it should be copy-pasted

dnolen17:06:34

it’s already pushed to master w/ tests

dnolen17:06:46

happy to get more eyes on it

favila17:06:15

@dnolen would be good to test when start is not 0, would capture any errors with forgetting start + n, indexes not being rewritten, viewing to left of subvec window but not vector, etc

favila17:06:23

(let [s (subvec [0 1 2 3 4] 1 3)]
  (testing "IFind"
    (is (= (find s 0) [0 1]))
    (is (= (find s 1) [1 2]))
    (is (= (find s 2) nil))
    (is (= (find s -1) nil))))

favila17:06:35

in fact I think this is going to fail

favila17:06:46

because we can't completely delegate -find

favila17:06:54

the index will be of the original, not the subvec

favila17:06:03

running test now

favila17:06:24

yeah, fails

favila17:06:19

(-find [coll n]
    (when (and (not (neg? n))
               (< (+ start n) end))
      (when-some [found (-find v (+ start n))]
        [(+ start (-key found)) (-val found)])))

favila17:06:46

start not n

dnolen17:06:51

that seems gratuitous? 🙂

dnolen17:06:22

[(+ start n) (-lookup v (+ start n))] no?

dnolen17:06:34

I think deferring -find is not correct

favila17:06:53

ah, if is between start and end, we know key exists

favila17:06:06

yes, that's better

favila17:06:12

no find delegation

favila17:06:47

ah but that's wrong too

favila17:06:59

[n (-lookup v (+ start n))]

dnolen17:06:34

good catch testing that now

favila17:06:00

passed tests for me

favila17:06:32

--- a/src/main/cljs/cljs/core.cljs
+++ b/src/main/cljs/cljs/core.cljs
@@ -5526,7 +5526,7 @@ reduces them without incurring seq initialization"
   (-find [coll n]
     (when (and (not (neg? n))
                (< (+ start n) end))
-      (-find v (+ start n))))
+      [n (-lookup v (+ start n))]))
 
   IVector
   (-assoc-n [coll n val]
diff --git a/src/test/cljs/cljs/collections_test.cljs b/src/test/cljs/cljs/collections_test.cljs
index 14dfa56..dfbfb82 100644
--- a/src/test/cljs/cljs/collections_test.cljs
+++ b/src/test/cljs/cljs/collections_test.cljs
@@ -99,10 +99,10 @@
         (testing "rseq equality"
           (is (= (rseq sv1) '(1)))
           (is (nil? (rseq sv2)))))
-      (let [s (subvec [0 1 2 3] 0 2)]
+      (let [s (subvec [0 1 2 3 4] 1 3)]
         (testing "IFind"
-          (is (= (find s 0) [0 0]))
-          (is (= (find s 1) [1 1]))
+          (is (= (find s 0) [0 1]))
+          (is (= (find s 1) [1 2]))
           (is (= (find s 2) nil))
           (is (= (find s -1) nil))))
       )

favila17:06:39

that's what I did

favila17:06:43

that passes

rauh18:06:08

Should IndexedSeq. get the IFind treatment? It implements IIndexed

favila18:06:28

It didn't occur to me that seqs were findable

rauh18:06:46

Yeah, you're right. Doesn't make sense for it.

dnolen18:06:41

@favila a fix applied to master + additional tests