core.rrb-vector

Add hash tests and hash fixes for Clojure 1.6

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

With the changes planned to hash functions in Clojure 1.6, it would be good to enhance core.rrb-vector so that it continues to work with Clojure 1.5.1, but is prepared to calculate hash functions on its data structures consistent with Clojure 1.6's changes, too.

  1. crrbv-3-v1.diff
    09/Feb/14 11:54 PM
    6 kB
    Andy Fingerhut
  2. crrbv-3-v1-test-only.diff
    16/Feb/14 11:50 AM
    3 kB
    Andy Fingerhut

Activity

Hide
Michał Marczyk added a comment -

My new-hasheq branch has been merged to master for a while now.

Also, I applied crrbv-3-v1-test-only.diff to master, then simplified it a bit in the next commit. Thanks!

Show
Michał Marczyk added a comment - My new-hasheq branch has been merged to master for a while now. Also, I applied crrbv-3-v1-test-only.diff to master, then simplified it a bit in the next commit. Thanks!
Hide
Andy Fingerhut added a comment -

Patch crrbv-3-v1-test-only.diff is identical to crrbv-3-v1.diff, except it only includes additions to the test suite run by 'mvn test', so that whatever problems it catches would be caught by the automated Maven builds done on clojure.org's build machine.

I have verified that with your new-hasheq branch changes and this patch, all of the tests pass with Clojure 1.5.1 and 1.6.0-beta1. Also that without your new-hasheq changes, these tests fail with 1.6.0-beta1, as they should.

Show
Andy Fingerhut added a comment - Patch crrbv-3-v1-test-only.diff is identical to crrbv-3-v1.diff, except it only includes additions to the test suite run by 'mvn test', so that whatever problems it catches would be caught by the automated Maven builds done on clojure.org's build machine. I have verified that with your new-hasheq branch changes and this patch, all of the tests pass with Clojure 1.5.1 and 1.6.0-beta1. Also that without your new-hasheq changes, these tests fail with 1.6.0-beta1, as they should.
Hide
Andy Fingerhut added a comment -

Looks fine to me. It would be nice to add some tests that are run automatically by the build server on every commit to Clojure, rather than having to discover it with a manually run test.

Show
Andy Fingerhut added a comment - Looks fine to me. It would be nice to add some tests that are run automatically by the build server on every commit to Clojure, rather than having to discover it with a manually run test.
Hide
Michał Marczyk added a comment -

Ok, seems to work now.

Show
Michał Marczyk added a comment - Ok, seems to work now.
Hide
Michał Marczyk added a comment -

...which is, by the way, broken because of new hashes of empty collections. Will fix in a moment.

Show
Michał Marczyk added a comment - ...which is, by the way, broken because of new hashes of empty collections. Will fix in a moment.
Hide
Michał Marczyk added a comment -

See https://github.com/clojure/core.rrb-vector/tree/new-hasheq for a commit using the new hash-ordered-coll.

Show
Michał Marczyk added a comment - See https://github.com/clojure/core.rrb-vector/tree/new-hasheq for a commit using the new hash-ordered-coll.
Hide
Andy Fingerhut added a comment -

Patch crrbv-3-v1.diff is one way to update the hash function for Clojure 1.6. I have tested it with the latest Clojure master as of Feb 8 2014, plus the current patches for these tickets:

CLJ-1339, CLJ-1331, CLJ-1335, CLJ-1336, CLJ-1344

It would be best to wait until the patches for these have been applied, and see if any changes are made to them that would affect this patch.

Show
Andy Fingerhut added a comment - Patch crrbv-3-v1.diff is one way to update the hash function for Clojure 1.6. I have tested it with the latest Clojure master as of Feb 8 2014, plus the current patches for these tickets: CLJ-1339, CLJ-1331, CLJ-1335, CLJ-1336, CLJ-1344 It would be best to wait until the patches for these have been applied, and see if any changes are made to them that would affect this patch.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: