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

@rauh does it matter?

I’d be very surprised if it does

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

what is emitted?

you need to be more clear 🙂

“emitted” like during profiling

or are you talking about codegen?

if codegen show an example

Codegen, yes! I'll give an example

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

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

@rauh that’s not useful 🙂

we need the ClojureScript source

I do understand what’s happening

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

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

Not so for function invokes (lists).

Does that help?

@rauh yep fixed in master

@dnolen Cool. Thanks.

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

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

seems very low priority 🙂

Ok seems fair 🙂

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

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

I believe there is, look in JIRA

Changes to IFind: https://dev.clojure.org/jira/browse/CLJS-2136

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

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

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

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

@favila no

this is for implementers

they will know when they mess it up

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

do what?

enumerate all assumptions - they do no such thing

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

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

etc

my answer is no

just remove the docstring changes

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

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

this just isn’t our problem

write tests, use generative tests, whatever

@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

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

I do not want the containment check subtlely in the docstring

Did you not notice this counterproposal?

@favila yes I misread what you said

assuming you want to keep the same docstring

I apologize

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

Ah, ok

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

That would satisfy me

go for it

Ok, thanks for slowing down, I appreaciate it

updating patch again

sorry for not reading what you said more closely

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

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

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

you coded those defensively

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

oh, let me double check

I think it was not ok

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

@tmulvaney what is going on with (assoc-n in collections-test 666?https://github.com/clojure/clojurescript/blob/master/src/test/cljs/cljs/collections_test.cljs#L666

just noticed the warning

is that meant to be -assoc-n?

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

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

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

it's much clearer with test-self-parity

test has a lot of noise about compiling files and timing

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

updating the patch

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

yeah I never use when/if-let anymore

Almost think they should be deprecated

many subtle bugs where false is an acceptable value

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

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

Does IFind on PersistentArrayMap have to be updated?

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

You've updated my map_entry_tests, cheers!

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

that's new the PAM

ah yeah missed that

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

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

oh is that still there?

hm

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

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

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.

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

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

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

@tmulvaney @favila let’s rewind

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

any validation of key must happen inside -find

and only fast validation is allowed

so if cannot validate quickly just leave it alone

@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

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

don’t care about float keys

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

no

because it’s not right either

your clamping to 32bit

it's enough to verify 32 bit int

we have vectors longer than that?

but vector size doesn’t have that limit

FWIW clojure does have a 32bit limit

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

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

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

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

huh

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

(that's Clojure)

so I appear to have inadvertently replicated clojure

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

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

caring and not caring - but this is about undefined behavior

@favila your patch looks OK, applied

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

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

I dropped the integer check bit in master

@favila Subvec seems wrong, on?

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

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

bah there are no subvec find tests

yeah the order of check is wrong

fixing

and adding a test

@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

so if you are intent on this remember to remove it

sorry not bounds check, int check

fixing

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

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

that’s what I’m about to commit

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

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

actually need to check not neg?

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

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

yeah

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

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

but @dnolen 's suggestion should be equivalent

Yup. Agreed

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

0 is not pos?

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

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

isn't end inclusive?

if not then -lookup is wrong

not inclusive

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

I mean the internal end

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

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

k

it’s already pushed to master w/ tests

happy to get more eyes on it

@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

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

in fact I think this is going to fail

because we can't completely delegate -find

the index will be of the original, not the subvec

running test now

yeah, fails

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

start not n

that seems gratuitous? 🙂

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

I think deferring -find is not correct

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

yes

yes, that's better

no find delegation

ah but that's wrong too

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

good catch testing that now

passed tests for me

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

that's what I did

that passes

Should IndexedSeq. get the IFind treatment? It implements IIndexed

It didn't occur to me that seqs were findable

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

@favila a fix applied to master + additional tests