ClojureScript

Bug in sort

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Environment:
    Mac OS X, recently cloned ClojureScript repository
  • Patch:
    Code and Test

Description

The sort function (and thus sort-by) do not seem to be behaving as they would in normal Clojure. For example, in my ClojureScript REPL I have:

ClojureScript:cljs.user> (def values (list [-11 10] [-8 12] [-15 -4] [4 5]))
([-11 10] [-8 12] [-15 -4] [4 5])
ClojureScript:cljs.user> (sort values)
([-11 10] [-15 -4] [-8 12] [4 5])

But in the Clojure REPL, you get what one would expect:
user=> (def values (list [-11 10] [-8 12] [-15 -4] [4 5]))
#'setback.core/values
user=> (sort values)
([-15 -4] [-11 10] [-8 12] [4 5])

I have only noticed this bug for sorting vectors, since in Clojure vectors are sorted position by position.

Activity

Hide
Sean Nilan added a comment -

The bug is actually in the compare function, which uses the google.array.defaultCompare as the compare function for vectors. A simple patch such as below could fix the problem:

(defn vector-compare
[v1 v2]
(cond
(and (empty? v1) (empty? v2))
0
(empty? v1)
-1
(empty? v2)
1
:default
(let [cmp (apply compare (map first [v1 v2]))]
(if (zero? cmp)
(vector-compare (rest v1) (rest v2))
cmp))))

(defn compare
"Comparator. Returns a negative number, zero, or a positive number
when x is logically 'less than', 'equal to', or 'greater than'
y. Uses google.array.defaultCompare for objects of the same type
and special-cases nil to be less than any other object."
[x y]
(cond
(and (vector? x) (vector? y)) (vector-compare x y)
(identical? (type x) (type y)) (garray/defaultCompare x y)
(nil? x) -1
(nil? y) 1
:else (throw (js/Error. "compare on non-nil objects of different types"))))

Show
Sean Nilan added a comment - The bug is actually in the compare function, which uses the google.array.defaultCompare as the compare function for vectors. A simple patch such as below could fix the problem: (defn vector-compare [v1 v2] (cond (and (empty? v1) (empty? v2)) 0 (empty? v1) -1 (empty? v2) 1 :default (let [cmp (apply compare (map first [v1 v2]))] (if (zero? cmp) (vector-compare (rest v1) (rest v2)) cmp)))) (defn compare "Comparator. Returns a negative number, zero, or a positive number when x is logically 'less than', 'equal to', or 'greater than' y. Uses google.array.defaultCompare for objects of the same type and special-cases nil to be less than any other object." [x y] (cond (and (vector? x) (vector? y)) (vector-compare x y) (identical? (type x) (type y)) (garray/defaultCompare x y) (nil? x) -1 (nil? y) 1 :else (throw (js/Error. "compare on non-nil objects of different types"))))
Hide
David Nolen added a comment -

Thanks. We need a IComparable protocol.

Show
David Nolen added a comment - Thanks. We need a IComparable protocol.
Hubert Iwaniuk made changes -
Field Original Value New Value
Assignee Hubert Iwaniuk [ neotyk ]
Hide
Hubert Iwaniuk added a comment -

IComparable protocol, implementations and tests.

Show
Hubert Iwaniuk added a comment - IComparable protocol, implementations and tests.
Hubert Iwaniuk made changes -
Attachment CLJS-204-IComparable.patch [ 11219 ]
Hubert Iwaniuk made changes -
Patch Code and Test [ 10002 ]
Hide
David Nolen added a comment -

Can we squash the commits here? Thanks!

Show
David Nolen added a comment - Can we squash the commits here? Thanks!
Hubert Iwaniuk made changes -
Attachment CLJS-204-IComparable.patch [ 11219 ]
Hide
Hubert Iwaniuk added a comment -

Commits squashed.

Show
Hubert Iwaniuk added a comment - Commits squashed.
Hubert Iwaniuk made changes -
Attachment CLJS-180-IComparable-squashed.patch [ 11220 ]
Hide
David Nolen added a comment -

Why do we need to extend IComparable to strings, arrays, boolean, and number?

Show
David Nolen added a comment - Why do we need to extend IComparable to strings, arrays, boolean, and number?
Hide
Hubert Iwaniuk added a comment -

booleans - are supported in clj
number - special case in clj, not sure if it brings any speed increase, will test with jsperf
arrays - are supported in clj
strings - to support symbols and keywords

Show
Hubert Iwaniuk added a comment - booleans - are supported in clj number - special case in clj, not sure if it brings any speed increase, will test with jsperf arrays - are supported in clj strings - to support symbols and keywords
Hide
Hubert Iwaniuk added a comment -

Simplified version, just to solve comparing vectors.

Show
Hubert Iwaniuk added a comment - Simplified version, just to solve comparing vectors.
Hubert Iwaniuk made changes -
Attachment CLJS-204-IComparable.patch [ 11227 ]
Hubert Iwaniuk made changes -
Attachment CLJS-180-IComparable-squashed.patch [ 11220 ]
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: