ClojureScript

-lookup method call inside get macro and keyword invoke

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • 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)))))

Activity

Hide
David Nolen added a comment -

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.

Show
David Nolen added a comment - 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.
Hide
Michał Marczyk added a comment -

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.

Show
Michał Marczyk added a comment - 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.
Hide
Jozef Wagner added a comment -

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.

Show
Jozef Wagner added a comment - 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.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated: