BTreeMultiset + iter specializations by Ten0 · Pull Request #27 · jmitchell/multiset

Ten0 · 2019-11-30T22:52:11Z

Resolves #25
Resolves #26

What it does:

BTreeMultiSet implementation
Specialize Iter for performance
Fix doc links

Due to GATs not being available in rust yet the MultiSet could not be generic on both types of map (could not create the Iter associated type because it is generic on a lifetime). In order to avoid code duplication, the code for the BTreeMultiset is generated at build time through a build.rs script, using simple replacements from the hash_multiset file.

The iterator I was able to factor however, so the specializations are only here once.

This is non semver-compatible due to the Iter type being more generic.

Only nth could be optimized further, feel free to implement it :)

mashedcode · 2019-12-01T23:08:57Z

Doing a quick diff hash_multiset.rs btree_multiset.rs. Everything in both files is the same except that BTreeMultiSet is used instead of HashMultiSet.
Which means whenever any change is made in the future it'll have to be made in both files. Wouldn't it be better to use a generic parameter for HashMultiSet instead to avoid code duplication?

Ten0 · 2019-12-01T23:12:36Z

If we could that would indeed be better but as I mentioned above unfortunately we need GATs to be able to output the iterator when making the MultiSet generic, which we can't have at the moment.

I've also experimented with a macro version (one liner for each sub-map) but it breaks the documentation and doesn't feel very clean.

mashedcode · 2019-12-01T23:25:50Z

Another approach if you'd like to avoid using macros is a build script that does a simple search and replace which produces the second file which should be in gitignore. If the docs are run after the build script it should even produce duplicate docs.

Cargo.toml

…icate code

mashedcode

Good job on the build.rs script.

Please remove the btree_multiset.rs file from 567aadb with a rebase.
I'd prefer it even more if you could split this PR into two separate ones. One for BTreeMultiset and one for the iterator stuff. But I guess it's fine like this.

src/iter.rs

build.rs

Cargo.toml

Ten0 · 2020-01-06T08:38:22Z

Please remove the btree_multiset.rs file from 567aadb

I agree it would hurt in the general history but I'd rather keep the PR history easily accessible along the discussion, so I think we should just squash and merge it.

# Conflicts: # src/hash_multiset.rs

Ten0 · 2020-02-22T11:09:52Z

Up

mashedcode

Thank you for addressing the review comments and adding test cases for the iterator!!

mashedcode · 2020-02-23T21:15:15Z

src/iter.rs

+            }
+            self.len -= 1;
+            Some(key)
+        } else {


clippy: this else { if .. } block can be collapsed to else if.

mashedcode · 2020-02-23T21:15:35Z

src/iter.rs

+            }
+            self.len -= 1;
+            Some(key)
+        } else {


clippy: this else { if .. } block can be collapsed to else if.

mashedcode · 2020-02-23T22:20:50Z

tests/specializations.rs

+        + BitXor<<IterItem as BitXor>::Output, Output = <IterItem as BitXor>::Output>
+        + Clone,
+    <IterItem as BitXor>::Output:
+        BitXor<Output = <IterItem as BitXor>::Output> + Eq + Debug + Clone,


It seems unnecessary to abstract this much and create so many single use functions. I wrote-up an example of a more specific implementation. (I took the freedom to use a macro for check_specialized.)

This comes from an implementation I had originally written for itertools, where I had several different iterators to test, so the functions were not single use. I just reused the same. ^^
Tbh, I hate writing tests, but I'm absolutely open to use whichever test implementation you prefer. :)

mashedcode · 2020-02-23T22:22:25Z

tests/specializations.rs

+    Iter: Iterator<Item = IterItem> + Clone + 'a,
+{
+    check_specialized(it, |mut i| {
+        let first = i.next().map(|f| f.clone() ^ (f.clone() ^ f));


Why since x ^ x ^ x == x? How about Default instead? Or even better yet 0i32?

That minimizes the amount of requirements on the generic function. Only output is constrained, so I need to hit a few XORs here to satisfy the type system.
Now again, if you'd rather use another implementation that doesn't try to minimize the constraints when writing the final test (and I agree this particular constraint probably wouldn't change anything), I'm absolutely open to it :)

mashedcode · 2020-02-23T22:25:18Z

tests/specializations.rs

+}
+
+quickcheck! {
+    fn hms_test_qc(test_vec: Vec<i32>) -> () {


What does "hms" stand for? hash multi set test? Or does it just test some iterator functionality?

Yes, hms stands for "hash multi set". qc is for quickcheck, a crate that allows to test on randomly generated inputs.

ssrlive · 2023-03-03T12:01:46Z

I need use the BTreeMultiset, I hope @jmitchell can merge this PR.

Ten0 changed the title ~~BTreeMultiset~~ BTreeMultiset + iter specializations Nov 30, 2019

Implement BTreeMultiset

567aadb

Ten0 force-pushed the btree-multiset branch 2 times, most recently from 1ae0269 to 7f86640 Compare December 1, 2019 00:08

Ten0 added 2 commits December 1, 2019 01:22

Specialize iterator for performance

e143c37

Only nth could be optimized further, feel free to implement it :)

Implement DoubleEndedIterator for Iter

8b3d25f

Ten0 force-pushed the btree-multiset branch from 7f86640 to 8b3d25f Compare December 1, 2019 00:24

Impl FusedIterator for Iter

c647e04

mashedcode reviewed Dec 1, 2019

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Implement BTreeMultiset through a build script instead of having dupl…

518ca32

…icate code

mashedcode requested changes Jan 5, 2020

View reviewed changes

src/iter.rs Outdated Show resolved Hide resolved

src/iter.rs Show resolved Hide resolved

build.rs Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

jmitchell mentioned this pull request Jan 6, 2020

RFC: project maintainership #28

Open

Ten0 added 5 commits January 11, 2020 14:35

Revert version bump

924b6ee

Indent build.rs with spaces

5d4c84d

Derive clone

0b28b68

Merge remote-tracking branch 'upstream/master' into btree-multiset

b11d3d6

# Conflicts: # src/hash_multiset.rs

Add specialization tests

7d68530

mashedcode requested changes Feb 23, 2020

View reviewed changes

Comments

Conversation

Ten0 commented Nov 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mashedcode commented Dec 1, 2019

Uh oh!

Ten0 commented Dec 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mashedcode commented Dec 1, 2019

Uh oh!

Uh oh!

mashedcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ten0 commented Jan 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ten0 commented Feb 22, 2020

Uh oh!

mashedcode left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mashedcode Feb 23, 2020

Choose a reason for hiding this comment

Uh oh!

mashedcode Feb 23, 2020

Choose a reason for hiding this comment

Uh oh!

mashedcode Feb 23, 2020

Choose a reason for hiding this comment

Uh oh!

Ten0 Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

mashedcode Feb 23, 2020

Choose a reason for hiding this comment

Uh oh!

Ten0 Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

mashedcode Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ten0 Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssrlive commented Mar 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ten0 commented Nov 30, 2019 •

edited

Loading

Ten0 commented Dec 1, 2019 •

edited

Loading

Ten0 commented Jan 6, 2020 •

edited

Loading

mashedcode left a comment •

edited

Loading

mashedcode Feb 23, 2020 •

edited

Loading

Ten0 Feb 24, 2020 •

edited

Loading