Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement BitOps for TrieSet #19518

Merged
merged 1 commit into from
Dec 5, 2014
Merged

Implement BitOps for TrieSet #19518

merged 1 commit into from
Dec 5, 2014

Conversation

csouth3
Copy link
Contributor

@csouth3 csouth3 commented Dec 4, 2014

Implement the BitOr, BitAnd, BitXor, and Sub traits from std::ops for TrieSet. The behavior of these operator overloads is consistent with RFC 235.

@alexcrichton
Copy link
Member

r? @gankro

Gankra
Gankra reviewed Dec 4, 2014
View reviewed changes
src/libcollections/trie/set.rs
@@ -463,6 +462,30 @@ impl Extend<uint> for TrieSet {
}
}

impl BitOr<TrieSet, TrieSet> for TrieSet {
fn bitor(&self, rhs: &TrieSet) -> TrieSet {
self.union(rhs).collect::<TrieSet>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the ::<TrieSet> annotation necessary? This should be easily inferred by the compiler (if it's not, we should be logging an issue).

@Gankra
Copy link
Contributor

Gankra commented Dec 4, 2014

implementation and tests are correct, just some style nits.

@csouth3
Copy link
Contributor Author

csouth3 commented Dec 4, 2014

@gankro OK, hopefully all of your comments are addressed! :)
I ended up fixing that error by switching back to use std::prelude::*; and then adding a use vec::Vec; and it worked like a charm.

@csouth3
Copy link
Contributor Author

csouth3 commented Dec 5, 2014

Cool. Looks like bit ops for BTreeSet are being taken care of presently, but I think I'll work on implementing them for TreeSet next.

@csouth3
Copy link
Contributor Author

csouth3 commented Dec 5, 2014

@gankro doctests and stability message added. Also, it looks like the old version of this is currently in the large rollup that's been trying to go through lately since it was r+'ed initially, so not sure how to proceed w.r.t. that.

@Gankra
Copy link
Contributor

Gankra commented Dec 5, 2014

I'll hash it out with cmr. Worst-case we can make the doc changes a seperate PR, but I don't think we'll need to.

@csouth3
Copy link
Contributor Author

csouth3 commented Dec 5, 2014

Cool, just let me know if I need to do anything further!

@Gankra
Copy link
Contributor

Gankra commented Dec 5, 2014

Just so I don't waste their time by breaking the rollup with this change: have you locally confirmed that the code in the docs checks out in our testing?

@csouth3
Copy link
Contributor Author

csouth3 commented Dec 5, 2014

Yes. make check-docs was happy with these. And I'm running the stuff I wrote for TreeSet through the same test right now.

@Gankra
Copy link
Contributor

Gankra commented Dec 5, 2014

Great, thanks!

@alexcrichton alexcrichton merged commit 309ab34 into rust-lang:master Dec 5, 2014
@csouth3 csouth3 deleted the trieset-bitops branch December 6, 2014 00:16
@japaric japaric mentioned this pull request Dec 6, 2014
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Gankra Gankra

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@csouth3 @alexcrichton @Gankra