<< Back to previous view

[CLJS-494] -lookup method call inside get macro and keyword invoke Created: 10/Apr/13  Updated: 14/Apr/13

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Daniel Skarda Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

ClojureScript from githhub



 Description   

I noticed that -lookup method is called incorrectly at two places

There are two -lookup methods defined for ILookup:

cljs/cljs/core.cljs:198
(defprotocol ILookup
(-lookup [o k] [o k not-found]))

clj/cljs/core.clj:426
(defmacro get
([coll k]
`(-lookup ~coll ~k nil))
([coll k not-found]
`(-lookup ~coll ~k ~not-found)))

As you see, macro (get foo bar) never calls first method. In first case it supplies default argument (instead calling method with 2 arguments).

Same for IFn and keywords:

(deftype Keyword [k]
IFn
(invoke [_ coll]
(when-not (nil? coll)
(let [strobj (.-strobj coll)]
(if (nil? strobj)
(-lookup coll k nil)
(aget strobj k)))))



 Comments   
Comment by Jozef Wagner [ 12/Apr/13 3:37 AM ]

Note that 2 argument get guarantees to return nil if the key is not found. This differs from e.g. 2 argument nth, which should throw if key is not found (index is out of bounds). If 2 argument -lookup does not guarantee to return nil when key is not found, we should keep it as it is.

Comment by MichaƂ Marczyk [ 12/Apr/13 5:13 AM ]

get insisting on nil is definitely by design, as explained by Jozef. So, there is no bug here.

As a matter of style it might be good to have get-the-function and get-the-macro do exactly the same thing; the question remains whether we should add an explicit nil to the -lookup call in get-the-function or remove the nil from the macro and just rely on -lookup to do the right thing. I'd probably go for the former to eliminate one unnecessary indirection (described below).

About -lookup "doing the right thing": it certainly seems to me that -lookup's contract is that of get, even if this is not explicitly documented; also, in almost all instances the binary -lookup delegates to ternary -lookup or ternary -nth (with nil supplied as the final argument), the exception being TransientHashMap's -lookup which basically has that call inlined.

Note that the binary -lookup is called is also called in some other places in core.cljs.

Comment by David Nolen [ 14/Apr/13 5:57 PM ]

Yes the main idea behind inlining get into -lookup was because they do essentially the same thing and we can remove a level of indirection. Similarly I'd prefer that we add the nil to -lookup to avoid the the indirection if not given a not-found value.

Generated at Fri Sep 19 10:51:32 CDT 2014 using JIRA 4.4#649-r158309.