Clojure

Add Comparable support to Vec

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.2
  • Component/s: None
  • Labels:
    None
  • Approval:
    Ok

Activity

Hide
Assembla Importer added a comment -

richhickey said: Parent association with ticket #264 was added

Show
Assembla Importer added a comment - richhickey said: Parent association with ticket #264 was added
Hide
Assembla Importer added a comment -

dsg said: This is my first attempt at submitting a real patch to Clojure, so I appreciate any feedback. This patch actually does a few different things, specifically:

1. Add java.lang.Comparable interface for Vec.
2. Modify vector-of to be a little more user-friendly. The original only creates empty Vecs. My patch allows vector-of to take arguments similar to vector and vec. Importantly, it also takes a primitive array as an argument. The implementation may not be the most efficient, but it seems to work.
3. I added a bunch of tests to ensure that vector-of mirrors the behaviour of standard vectors. In doing so, I ran into a few bugs that led me to implement the Associative interface for Vec and IPersistentCollection to VecSeq.

It is still probably a good idea to add more tests.

Show
Assembla Importer added a comment - dsg said: This is my first attempt at submitting a real patch to Clojure, so I appreciate any feedback. This patch actually does a few different things, specifically: 1. Add java.lang.Comparable interface for Vec. 2. Modify vector-of to be a little more user-friendly. The original only creates empty Vecs. My patch allows vector-of to take arguments similar to vector and vec. Importantly, it also takes a primitive array as an argument. The implementation may not be the most efficient, but it seems to work. 3. I added a bunch of tests to ensure that vector-of mirrors the behaviour of standard vectors. In doing so, I ran into a few bugs that led me to implement the Associative interface for Vec and IPersistentCollection to VecSeq. It is still probably a good idea to add more tests.
Hide
Assembla Importer added a comment -

dsg said: [file:aUPsnCrXyr36i1eJe5avMc]: Patch to fix #266

Show
Assembla Importer added a comment - dsg said: [file:aUPsnCrXyr36i1eJe5avMc]: Patch to fix #266
Hide
Assembla Importer added a comment -

stu said: Hi Daniel,

Thanks for jumping on this. I am reviewing the patch, and have hit one issue so far. It's a subtle one:

(let [foo (into (vector-of :int) [1 2 3 4])]
  (println "count: "(count (seq (next foo))))
  (println ".count: " (.count (seq (next foo)))))

Your count isn't taking account of the offset in the VecSeq. This is hidden by a weakness in Clojure's count fn: it doesn't know when part of a seq is counted to take advantage of that, so you can't even see the issue by calling count!

This makes me think we need to test every Java interface method directly.

Show
Assembla Importer added a comment - stu said: Hi Daniel, Thanks for jumping on this. I am reviewing the patch, and have hit one issue so far. It's a subtle one:
(let [foo (into (vector-of :int) [1 2 3 4])]
  (println "count: "(count (seq (next foo))))
  (println ".count: " (.count (seq (next foo)))))
Your count isn't taking account of the offset in the VecSeq. This is hidden by a weakness in Clojure's count fn: it doesn't know when part of a seq is counted to take advantage of that, so you can't even see the issue by calling count! This makes me think we need to test every Java interface method directly.
Hide
Assembla Importer added a comment -

dsg said: Parent association with ticket #297 was added

Show
Assembla Importer added a comment - dsg said: Parent association with ticket #297 was added
Hide
Assembla Importer added a comment -

dsg said: [file:bt20JetxCr36GpeJe5d-aX]: Adds Comparable support to Vec, with tests.

Show
Assembla Importer added a comment - dsg said: [file:bt20JetxCr36GpeJe5d-aX]: Adds Comparable support to Vec, with tests.
Hide
Assembla Importer added a comment -

dsg said: New patch for Comparable support. I'll move the other fixes/features to other bugs. This new patch (dated 21 Apr), also includes tests specifically for Comparable.

Show
Assembla Importer added a comment - dsg said: New patch for Comparable support. I'll move the other fixes/features to other bugs. This new patch (dated 21 Apr), also includes tests specifically for Comparable.
Hide
Assembla Importer added a comment -

stu said: Hi Daniel,

This looks great. One tweak: it is not idiomatic (in Clojure at least) to do the type check. All the other Comparable types in Clojure simply throw a ClassCastException when you pass a bad arg to .compareTo.

If you convert that check to a Clojure (cast ...) and update the tests, I think we're good to go.

(I can also make this change, but I want to let the whole thing go in under your name.)

Cheers,
Stu

Show
Assembla Importer added a comment - stu said: Hi Daniel, This looks great. One tweak: it is not idiomatic (in Clojure at least) to do the type check. All the other Comparable types in Clojure simply throw a ClassCastException when you pass a bad arg to .compareTo. If you convert that check to a Clojure (cast ...) and update the tests, I think we're good to go. (I can also make this change, but I want to let the whole thing go in under your name.) Cheers, Stu
Hide
Assembla Importer added a comment -

dsg said: Hello, Stu,

Thanks for the feedback. I'll make your suggested changes and open those other tickets tomorrow.

Sincerely,

Daniel Solano G��mez

Show
Assembla Importer added a comment - dsg said: Hello, Stu, Thanks for the feedback. I'll make your suggested changes and open those other tickets tomorrow. Sincerely, Daniel Solano G��mez
Hide
Assembla Importer added a comment -

dsg said: [file:c2n-HUtXWr34oReJe5dVir]: Revised patch to add Comparable support to Vec.

Show
Assembla Importer added a comment - dsg said: [file:c2n-HUtXWr34oReJe5dVir]: Revised patch to add Comparable support to Vec.
Hide
Assembla Importer added a comment -

dsg said: I have submitted a new patch 'add_comparable_support_to_vec-revised.patch', which includes Stu���s recommendations and adds a couple of extra tests.

Show
Assembla Importer added a comment - dsg said: I have submitted a new patch 'add_comparable_support_to_vec-revised.patch', which includes Stu���s recommendations and adds a couple of extra tests.
Hide
Assembla Importer added a comment -
Show
Assembla Importer added a comment - stu said: [file:d_AO6it8Sr37m4eJe5dVir]
Hide
Assembla Importer added a comment -

stu said: recommend the apr 23 patch (which solves the problem) plus the apr 24 patch (which removes reflection from the api)

Show
Assembla Importer added a comment - stu said: recommend the apr 23 patch (which solves the problem) plus the apr 24 patch (which removes reflection from the api)
Hide
Assembla Importer added a comment -

importer said: (In [[r:a3d1d494e9e574599c50dd83749183c66f653192]]) Add Comparable support to Vec, with tests. Fixes #266.

Signed-off-by: Stuart Halloway <stu@thinkrelevance.com>

Branch: master

Show
Assembla Importer added a comment - importer said: (In [[r:a3d1d494e9e574599c50dd83749183c66f653192]]) Add Comparable support to Vec, with tests. Fixes #266. Signed-off-by: Stuart Halloway <stu@thinkrelevance.com> Branch: master

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: