Add BitOps for BTreeSet. #19514
Add BitOps for BTreeSet. #19514
Conversation
|
If you want to stick to that implementation strategy, you'll need to bind |
|
That said, I'm not convinced that this is actually desirable, and no other collections overload operators like this. cc @aturon what are your thoughts on using |
|
I am working on this based on the Set Operations: Combinations section in the RFC. Also, this. /cc @reem |
|
@reem The use of overloaded operations for sets was part of the collections reform RFC. Here are my thoughts. Part of the motivation there is that there are actually several ways you may want to combine sets -- you can do so to get an iterator, you can do so to get a new set, or you can mutate a set in place. Each of these wants a specialized implementation for performance reasons, but it's a natural fit to make e.g. Further, this use of the operators is very consistent with their normal mathematical usage, so it seems like a fairly unobjectionable use of overloading to me. |
|
Ok, that seems perfectly reasonable. I just hadn't read that part of the RFC so was confused. |
| @@ -340,6 +340,12 @@ impl<T: Ord> Default for BTreeSet<T> { | |||
| } | |||
| } | |||
|
|
|||
| impl<T: Ord> BitOr<BTreeSet<T>,BTreeSet<T>> for BTreeSet<T> { | |||
| fn bitor(&self, _rhs: &BTreeSet<T>) -> BTreeSet<T> { | |||
| self.union(_rhs).map(|&i| i).collect::<BTreeSet<T>>() | |||
There was a problem hiding this comment.
self.union(_rhs).cloned().collect() (without the BTreeSet annotation on collect, it can be inferred) should do the job. But yes, you'll need to add the Clone constraint to T.
There was a problem hiding this comment.
I would remove the _ from rhs. We usually use that to indicate the value isn't used, which is clearly not true here.
There was a problem hiding this comment.
@gankro I am not sure what you mean by adding the Clone constraint to T. Could you elaborate a bit more on that?
There was a problem hiding this comment.
Clone is the trait that allows values to be deep-copied. You need to specify that T implements Clone, because after this operation there should be two logical copies of some values. For instance, if T was a Box, just moving out of a reference would create two Boxes that point to the same location in heap. Not safe. Clone will duplicate the actual location in memory, making two independent boxes.
You can add the bound as follows:
impl<T: Ord + Clone> BitOr<BTreeSet<T>,BTreeSet<T>> for BTreeSet<T> {
There was a problem hiding this comment.
That clears it up a lot. Thanks for explaining that! 😄
|
Basic implementation strategy is correct, just need my two notes to get it production-ready. |
jbranchaud
changed the title
|
Hi! Just a couple of things I see (though as a disclaimer I'm a complete newbie here myself, so take with a grain of salt!). First, it's probably good to remove the FIXME at the top of the file for implementing these traits as a part of this PR since you have taken care of implementing them. Also, it may be desirable to roll these commits into one to keep the Git commit history clean, though I'd wait for official word from Gankro on that one since it may not be necessary. Luckily enough, Git makes that easy to do though :) Cheers, and good work! |
|
Yeah everything csouth said is correct. Kill the FIXME, and eventually squash the commits when we're done. However you should include some tests for this functionality. In addition it just occurred to me that you should document the trait implementations and mark them with the collections reform "unstable" marker. See #19561 for dicussion. |
jbranchaud
force-pushed
the
add-btree-set-bitor
branch
from
6de4336 to
71c8e7a
Compare
|
@jbranchaud What's the status of this? Looks like it should be ready for the buildbot with the addition of appropriate documentation/doctests and unit tests :) |
|
Doc tests should be sufficient as far as unit tests go (the implementations aren't exactly difficult to verify). @jbranchaud You can steal the appropriate docs from TreeSet. |
Add initial attempt at implementing BitOr for BTreeSet. Update the implementation of the bitor operator for BTreeSets. `make check` ran fine through this. Add implementations for BitAnd, BitXor, and Sub as well. Remove the FIXME comment and add unstable flags. Add doctests for the bitop functions.
jbranchaud
force-pushed
the
add-btree-set-bitor
branch
from
71c8e7a to
cd008c4
Compare
|
This should be ready to go. I'm glad to address any other comments though 😃 |
I am trying to add an implementation of `bitor` for `BTreeSet`. I think I am most of the way there, but I am going to need some guidance to take it all the way.
When I run `make check`, I get:
```
error: cannot move out of dereference of `&`-pointer
self.union(_rhs).map(|&i| i).collect::<BTreeSet<T>>()
^~
```
I'd appreciate any nudges in the right direction. If I can figure this one out, I am sure I will be able to implement `bitand`, `bitxor`, and `sub` as well.
/cc @gankro
---
**Update**
I have added implementations for `BitOr`, `BitAnd`, `BitXor`, and `Sub` for `BTreeSet`.
This fixes rust-lang#19514 using a two-line patch suggested by @lifthrasiir. The code snippet: ```rust extern crate serialize; fn main() { let s = serialize::json::encode(&'c'); println!("Encoded: {}, bytes: {}", s, s.as_bytes()); } ``` ...serialized 'c' as `[34, 99, 0, 0, 0, 34]` when it should have used `[34, 99, 34]`. A test case is included.
…s-string Fix color brackets in string context
I am trying to add an implementation of
bitorforBTreeSet. I think I am most of the way there, but I am going to need some guidance to take it all the way.When I run
make check, I get:I'd appreciate any nudges in the right direction. If I can figure this one out, I am sure I will be able to implement
bitand,bitxor, andsubas well./cc @gankro
Update
I have added implementations for
BitOr,BitAnd,BitXor, andSubforBTreeSet.