<< Back to previous view

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

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

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

ClojureScript from githhub


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

There are two -lookup methods defined for ILookup:

(defprotocol ILookup
(-lookup [o k] [o k not-found]))

(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]
(invoke [_ coll]
(when-not (nil? coll)
(let [strobj (.-strobj coll)]
(if (nil? strobj)
(-lookup coll k nil)
(aget strobj k)))))

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.

Comment by David Nolen [ 21/Feb/15 7:42 AM ]

This is not a bug.

Generated at Wed Jan 17 16:25:15 CST 2018 using JIRA 4.4#649-r158309.