<< Back to previous view

[CLJ-1128] Improve merge-with Created: 13/Dec/12  Updated: 13/Dec/12

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Edward Tsech Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Improve-merge-with.patch     Text File 0002-Improve-merge-with.patch    
Patch: Code

 Description   

Set a first map as an initial value for reduce to
avoid merge-entry (series of contains? calls and etc) call on the first map.



 Comments   
Comment by Edward Tsech [ 13/Dec/12 1:36 PM ]

Tests pass.

Comment by Andy Fingerhut [ 13/Dec/12 2:32 PM ]

Edward, your patch replaces the expression (or m1 {}) with m1. It was changed from m1 to (or m1 {}) in a commit on Oct 16, 2008 with descriptive text "improved nil handling in merge, merge-with", so I am pretty sure it would be best to leave it as (or m1 {}). I believe the intent is to allow all but one of the map arguments to merge-with be nil, and everything will still work.

The rest of the patch for avoiding one merge call seems sound to me.

Your change would be even better at preserving any metadata on the first non-nil map in the list, if instead of calling with the first map, it called it with the first non-nil item of the list, and then the rest of the list after that.

Comment by Edward Tsech [ 13/Dec/12 5:41 PM ]

I figured out that `reduce1` did pass a head of the list for me. (https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L887)
But case with first nil argument is still valid. Correct me, please, if i'm wrong.

I'm not sure about `(or m1 {})`. I don't see any problems which can happen. Probably behaviour of functions which are used internally was changed since 2008.

(contains? nil :a) ;=> false
(assoc nil :a 1) ;=> {:a 1}
(get nil :a) ;=> nil

I could write some tests for that.

Generated at Wed Dec 17 15:37:05 CST 2014 using JIRA 4.4#649-r158309.