Add named functions to perform set operations #244
Conversation
| /// This is equivalent to using the `&` operator (e.g. | ||
| /// [`ops::BitAnd`]), as in `flags & other`. | ||
| /// | ||
| /// [`ops::BitAnd`]: https://doc.rust-lang.org/std/ops/trait.BitAnd.html |
There was a problem hiding this comment.
I asked around and there's
- No great way to link to the implementation of a trait for
Self. - Not really a good way to link to right location of the trait in all cases, especially when cases like
feature = "rustc-dep-of-std"are considered.
Note that broken links produces a warning (in some more recent versions of rust), and that warning would appear in the users crate, so it seems better to just explicitly give the link, rather than trying to make intra-doc-links figure it out.
| } | ||
|
|
||
| #[test] | ||
| fn test_set_ops_exhaustive() { |
There was a problem hiding this comment.
This should also improve our test coverage of the ops::Foo implementations a lot, at least in terms of edge cases.
| } | ||
| } | ||
| let iter_test_flags = | ||
| || (0..=0b111_1111_1111).map(|bits| unsafe { Test::from_bits_unchecked(bits) }); |
There was a problem hiding this comment.
This is a lot of test cases, especially given that the test iterates over it twice (nested), but even for a debug build, it completes in under half a second on a relatively underpowered machine. This is fast enough not to be noticeable at all (that is, the tests don't stall on it, waiting for it to complete).
It probably helps that all it's doing is verifying the result of some bitwise operations (but also: computers are fast).
| @@ -653,7 +665,6 @@ macro_rules! __impl_bitflags { | |||
| pub const fn contains(&self, other: $BitFlags) -> bool { | |||
| (self.bits & other.bits) == other.bits | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Ugh. This line had some trailing whitespace on it, and while I was pretty careful to try to keep this diff clean (... y'all kinda need to run rustfmt, at least arguably), I guess this one slipped through the cracks.
LMK if you'd like me to readd the trailing whitespace that used to be here.
|
It's not really clear to me if this should be considered a breaking change or not... In theory it is, since it's possible people do I also could do it as an off-by-default feature, which wouldn't technically fix the issue, but could make it so that nobody who isn't opting into this has to deal with any kind of breakage. Edit: Another (decidedly worse most ways) option that's less likely to have breakage is to use the operator names for the This solves 3, 1, but definitely not 2 at all. But it probably wouldn't cause breakage, since it's such a weird thing to do (impl an inherent method with the same name as a operator trait method), that it's even less. It would definitely confuse people though, and I... honestly kinda hate the thought of e.g. using " |
|
Wow thanks for working on this @thomcc! This is a really thoroughly considered addition. As for whether or not it's a breaking change, we're in a bit of a tricky position here with adding inherent methods to types defined by In general I think we should avoid adding inherent methods, but for this specific case I think we could justify adding these methods as they could have reasonably been included pre 1.0, have a single obvious definition, and complement inherent methods that already exist. If there are any other methods that would also fit into this category we might want to include them here too. Can you think of any we should consider? I can't off the top of my head. |
|
The only other one I can think of is a |
I can't think of anything else, no. |
|
In the future we may want to replace all the |
|
Alrighty, it looks like we're building up some nice content for a release, so I'll merge this one in and we can take stock of what's on |
There are a few reasons to do this:
Tooling and documentation is much better about exposing inherent methods than trait implementations. This is true for rustdoc, but it also is (inherently) the case for autocomplete (I can do
thing.and see all the methods available for a type, but it won't show me operator overloads)Somewhat subjective, but IMO there are situations where these names are clearer than using bitwise operators. They certainly are much easier to find documentation for, and it's more obvious at a glance that you aren't working with e.g. plain integers.
It unblocks the ability to perform these operations to initialize
consts/statics and withinconst fn. I gather that the stance of this repo in the past been to wait for the operator traits to be usable in CTFE contexts, but this seems extremely far off from stabilizing (I asked about this on zulip, and they confirmed that my impression is correct).For full disclosure: this is a lot of my motivation here, but I also do genuinely believe what I wrote for 1 and 2 (that is, I'd like these as part of the API, but probably would not have bothered to write the patch if it were not for this).