<< Back to previous view

[CLJS-793] memoize doesn't correctly cache non-truthy return values Created: 08/Apr/14  Updated: 18/Apr/14  Resolved: 18/Apr/14

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

Type: Defect Priority: Minor
Reporter: Peter Taoussanis Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-793.patch     Text File CLJS-793.patch     Text File CLJS-793.patch     Text File patch_commit_2ba4dac94918.patch    
Patch: Code


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.

Comment by David Nolen [ 08/Apr/14 9:23 AM ]

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

Comment by Peter Taoussanis [ 08/Apr/14 9:41 AM ]

Updated patch formatting, thanks!

Comment by David Nolen [ 08/Apr/14 10:55 AM ]

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.

Comment by Peter Taoussanis [ 08/Apr/14 11:01 AM ]

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

Comment by David Nolen [ 08/Apr/14 7:37 PM ]

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.

Comment by Peter Taoussanis [ 09/Apr/14 12:18 AM ]

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!


Comment by David Nolen [ 10/Apr/14 12:23 PM ]

Looks good thanks!

Comment by David Nolen [ 18/Apr/14 8:32 AM ]

fixed https://github.com/clojure/clojurescript/commit/4bbbd6068af766ab195de99ba07f54dd87e576c0

Generated at Wed Jan 17 22:04:29 CST 2018 using JIRA 4.4#649-r158309.