[CLJS-494] -lookup method call inside get macro and keyword invoke Created: 10/Apr/13 Updated: 14/Apr/13
ClojureScript from githhub
I noticed that -lookup method is called incorrectly at two places
There are two -lookup methods defined for ILookup:
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]
|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.