Clojure

Converted from http://www.assembla.com/spaces/clojure/tickets/266
Attachments:
fix266.patch - https://www.assembla.com/spaces/clojure/documents/aUPsnCrXyr36i1eJe5avMc/download/aUPsnCrXyr36i1eJe5avMc
add_comparable_support_to_vec.patch - https://www.assembla.com/spaces/clojure/documents/bt20JetxCr36GpeJe5d-aX/download/bt20JetxCr36GpeJe5d-aX
add_comparable_support_to_vec-revised.patch - https://www.assembla.com/spaces/clojure/documents/c2n-HUtXWr34oReJe5dVir/download/c2n-HUtXWr34oReJe5dVir
eliminate-reflection-in-266.patch - https://www.assembla.com/spaces/clojure/documents/d_AO6it8Sr37m4eJe5dVir/download/d_AO6it8Sr37m4eJe5dVir

richhickey said: Parent association with ticket #264 was added

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.

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

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.

dsg said: Parent association with ticket #297 was added

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

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.

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

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

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

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.

stu said: [file:d_AO6it8Sr37m4eJe5dVir]

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

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

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

Branch: master