ClojureScript

get should accept any type the same way Clojure JVM does

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

The following throws instead of returning nil:

(get 42 :anything)

Activity

Hide
Michał Marczyk added a comment -

clojure.lang.RT-style fix.

From the commit message:

The fix involves changes to both core.cljs and core.clj, since get is backed by a macro. Tests are introduced for both direct and higher-order calls.

Show
Michał Marczyk added a comment - clojure.lang.RT-style fix. From the commit message: The fix involves changes to both core.cljs and core.clj, since get is backed by a macro. Tests are introduced for both direct and higher-order calls.
Hide
Michał Marczyk added a comment -

Incidentally, I've prepared the patch on top of patches for CLJS-491 and CLJS-492, it should still apply to master though.

Show
Michał Marczyk added a comment - Incidentally, I've prepared the patch on top of patches for CLJS-491 and CLJS-492, it should still apply to master though.
Hide
David Nolen added a comment -

I'd prefer that the macro not emit a conditional check. I think it's probably OK if we just provide a default case for ILookup. Thanks.

Show
David Nolen added a comment - I'd prefer that the macro not emit a conditional check. I think it's probably OK if we just provide a default case for ILookup. Thanks.
Hide
Michał Marczyk added a comment -

Sure, here's a new patch, with a default case provided for ILookup and the tests from the previous patch. Both macro and function unchanged, since with this change everything satisfies? ILookup.

Out of curiosity, why the preference? Do you think this sort of expansion jumping in at the wrong place could pose problems for GClosure?

Show
Michał Marczyk added a comment - Sure, here's a new patch, with a default case provided for ILookup and the tests from the previous patch. Both macro and function unchanged, since with this change everything satisfies? ILookup. Out of curiosity, why the preference? Do you think this sort of expansion jumping in at the wrong place could pose problems for GClosure?
Hide
David Nolen added a comment -

Thanks get is just extremely common and likely to appear in expression cases - where the CLJS compiler will emit a function. If these get too nested GClosure won't handle them and it's a pretty big perf hit.

Show
David Nolen added a comment - Thanks get is just extremely common and likely to appear in expression cases - where the CLJS compiler will emit a function. If these get too nested GClosure won't handle them and it's a pretty big perf hit.
Hide
Michał Marczyk added a comment -

Right, thanks.

Show
Michał Marczyk added a comment - Right, thanks.

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: