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.

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

Add named functions to perform set operations #244

Merged
merged 1 commit into from Aug 3, 2021

Conversation

thomcc
Copy link
Contributor

@thomcc thomcc commented Jun 10, 2021

There are a few reasons to do this:

  1. 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)

  2. 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.

  3. It unblocks the ability to perform these operations to initialize consts/statics and within const 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).

/// 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
Copy link
Contributor Author

@thomcc thomcc Jun 10, 2021

Choose a reason for hiding this comment

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

I asked around and there's

  1. No great way to link to the implementation of a trait for Self.
  2. 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() {
Copy link
Contributor Author

@thomcc thomcc Jun 10, 2021

Choose a reason for hiding this comment

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

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) });
Copy link
Contributor Author

@thomcc thomcc Jun 10, 2021

Choose a reason for hiding this comment

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

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
}

Copy link
Contributor Author

@thomcc thomcc Jun 10, 2021

Choose a reason for hiding this comment

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

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.

@thomcc
Copy link
Contributor Author

thomcc commented Jun 10, 2021

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 impl Blah after defining Blah with bitflags!, but I dunno how likely it is that they've defined these functions. AFAICT (From some grep.app sleuthing) I can't find anything, but that doesn't mean it doesn't exist.

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 const fn inherent methods.

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 "a.bitxor(b)" to perform the set-theoretic symmetric difference.

@KodrAus
Copy link
Member

KodrAus commented Jul 19, 2021

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 bitflags because even though we define them we don't technically own them. Normally we'd solve this by defining a trait and using that, but that's not an option here because we can't make those trait methods const. The off-by-default feature might help us feel a little better, but in practice it wouldn't prevent breakage in crates already defining these methods without opting in to the feature since some other dependency in the graph might do it for them.

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.

@KodrAus
Copy link
Member

KodrAus commented Jul 19, 2021

The only other one I can think of is a const default based on #205, but that's out-of-scope for this PR.

@thomcc
Copy link
Contributor Author

thomcc commented Jul 19, 2021

Can you think of any we should consider? I can't off the top of my head.

I can't think of anything else, no.

src/lib.rs Show resolved Hide resolved
@konsumlamm
Copy link
Contributor

konsumlamm commented Jul 19, 2021

In the future we may want to replace all the $BitFlags in the impl blocks with Self, but that's out of scope for this PR. I can put together a PR for that after this is merged.

@KodrAus
Copy link
Member

KodrAus commented Aug 3, 2021

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 main.

@KodrAus KodrAus merged commit ec0fa76 into bitflags:master Aug 3, 2021
@konsumlamm konsumlamm mentioned this pull request Aug 4, 2021
@KodrAus KodrAus mentioned this pull request Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants