Clojure

Improve merge-with

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • 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.

Activity

Descending order - Click to sort in ascending order
Hide
Permalink
Edward Tsech added a comment -

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.

Show
Edward Tsech added a comment - 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.
Hide
Permalink
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Permalink
Edward Tsech added a comment -

Tests pass.

Show
Edward Tsech added a comment - Tests pass.

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.