ClojureScript

memoize doesn't correctly cache non-truthy return values

Details

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

Description

ClojureScript's memoize fn currently uses `(get @mem args)` to check for the existence of a cache entry. This prevents falsey values from being cached correctly.

A direct copy of Clojure's `memoize` code fixes this, patch attached.

This is the first issue+patch I've submitted, so please double check for mistakes - thanks.

  1. CLJS-793.patch
    09/Apr/14 12:18 AM
    1 kB
    Peter Taoussanis
  2. CLJS-793.patch
    08/Apr/14 11:01 AM
    1 kB
    Peter Taoussanis
  3. CLJS-793.patch
    08/Apr/14 9:41 AM
    0.8 kB
    Peter Taoussanis
  4. patch_commit_2ba4dac94918.patch
    08/Apr/14 2:18 AM
    0.5 kB
    Peter Taoussanis

Activity

Hide
David Nolen added a comment -

The patch is not properly formatted - please follow these instructions http://github.com/clojure/clojurescript/wiki/Patches

Show
David Nolen added a comment - The patch is not properly formatted - please follow these instructions http://github.com/clojure/clojurescript/wiki/Patches
Hide
Peter Taoussanis added a comment -

Updated patch formatting, thanks!

Show
Peter Taoussanis added a comment - Updated patch formatting, thanks!
Peter Taoussanis made changes -
Field Original Value New Value
Attachment CLJS-793.patch [ 12925 ]
Hide
David Nolen added a comment -

Thanks, I've updated the instructions to be clearer on what the commit message should look like. We've been inconsistent in the past and I would like for people to follow a basic guideline. Thanks.

Show
David Nolen added a comment - Thanks, I've updated the instructions to be clearer on what the commit message should look like. We've been inconsistent in the past and I would like for people to follow a basic guideline. Thanks.
Hide
Peter Taoussanis added a comment -

No problem, thanks! Just updated to fit the new spec exactly.

Show
Peter Taoussanis added a comment - No problem, thanks! Just updated to fit the new spec exactly.
Peter Taoussanis made changes -
Attachment CLJS-793.patch [ 12927 ]
Hide
David Nolen added a comment -

Looking at this more closely I would prefer that this be done with a closed over sentinel value via (js-obj), and to perform an identical? check on (get @mem args sentinel) to see if it matches the sentinel.

Show
David Nolen added a comment - Looking at this more closely I would prefer that this be done with a closed over sentinel value via (js-obj), and to perform an identical? check on (get @mem args sentinel) to see if it matches the sentinel.
Hide
Peter Taoussanis added a comment -

Sure, updated the patch. Please note that I used the global `lookup-sentinel` instead of closing over a new one. Let me know if there was some reason you wanted a unique sentinel & I'll update!

Cheers

Show
Peter Taoussanis added a comment - Sure, updated the patch. Please note that I used the global `lookup-sentinel` instead of closing over a new one. Let me know if there was some reason you wanted a unique sentinel & I'll update! Cheers
Peter Taoussanis made changes -
Attachment CLJS-793.patch [ 12929 ]
Hide
David Nolen added a comment -

Looks good thanks!

Show
David Nolen added a comment - Looks good thanks!
David Nolen made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Resolved [ 5 ]
David Nolen made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: