BitFlags trait #220

arturoc · 2020-06-18T14:45:04Z

Adds a BitFlags trait that is implemented by every bitflags.

The trait includes documentation so it the current hack to document methods through an example could be removed

Closes #154

Adds a BitFlags trait that is implemented by every bitflags. The trait includes documentation so it the current hack to document methods through an example could be removed Closes bitflags#154

Adam-Gleave · 2021-03-13T10:49:30Z

Any updates on this? I've been looking for something like this for a project, and the changes look reasonable.

KodrAus · 2021-05-16T05:33:41Z

src/bitflags_trait.rs

+use core as _core;
+
+pub trait BitFlags<T>:
+    _core::ops::BitOr<Output = Self>


I think it might be better for the BitFlags trait not to include these additional supertraits and let consumers add them as additional bounds if they need them.

On the one hand it seems reasonable that if you've got flags you'll want to perform bitwise operations on them, but on the other it's a bit of a grab-bag snapshot of our current functionality that we won't be able to extend (since we can't add new supertraits without breakage).

It might be best to stick with just the current set of methods on the trait which cover:

Construction from bits

Conversion back into bits

Set operations on pairs of flags

That should be enough to support a wide range of fundamental use-cases.

What about Clone + Copy? I need those to implement the iterator which is based on a value and removes the flags as it advances through it.

Do we need the BitFlags type itself to be Copy? Or just the underlying bits? If I understand right, we could just perform shifts and masks on the bits themselves and use BitFlags::from_bits_unchecked to return just that flag.

KodrAus · 2021-05-16T05:33:52Z

src/bitflags_trait.rs

+    fn from_bits(bits: T) -> _core::option::Option<Self>;
+    /// Convert from underlying bit representation, dropping any bits
+    /// that do not correspond to flags.
+    unsafe fn from_bits_truncate(bits: T) -> Self;


This method doesn't need to be unsafe.

KodrAus · 2021-05-16T05:34:28Z

Thanks for your patience on this one everybody! I've had a look through this now and left some thoughts on what direction we might want to take it in 🙇

KodrAus · 2021-05-16T05:50:16Z

src/bitflags_trait.rs

@@ -0,0 +1,68 @@
+use core as _core;
+
+pub trait BitFlags<T>:


We should probably make this an associated type, since an implementation of BitFlags only corresponds to a single underlying integer type.

I think the lines in this comment are wrong? do you mean perhaps empty and all?

Ah sorry, I mean making the trait look like:

pub trait BitFlags { type Bits; .. }

instead of:

pub trait BitFlags<T> { .. }

…tflags_trait

arturoc · 2021-05-17T08:26:34Z

I've implemented all the changes and left out Copy and Clone by now since they are not really needed for this PR, once this is merged I'll look into what's the minimum needed for the iterators PR.

Two of the tests are failing but doesn't seem related to this, the 1.32 test seems to be related to unsupported features in const function because that function is not wrapped with __fn_bitflags! I'll send another PR to fix that. The nightly tests are failing cause the output format has changed so the comparison with the generated private_flags.stderr is not equal anymore

bradleyharden · 2021-06-25T02:13:08Z

@arturoc, we ran into a case where this would be useful for the atsamd-hal crate. I'm looking forward to seeing it get merged. Thanks for working on it.

KodrAus · 2021-08-03T23:14:29Z

Hey @bradleyharden! 👋

Would you be happy to share some more details of your usecase? Would the trait in its current shape without any supertraits be sufficient for you?

KodrAus · 2021-08-03T23:16:20Z

Looks like we just need a rebase on the current main, which should fix up those build failures.

Thanks again for persisting with this @arturoc

konsumlamm · 2021-08-04T01:10:17Z

src/bitflags_trait.rs

@@ -0,0 +1,46 @@
+use core as _core;


Suggested change

use core as _core;

This is only necessary for use in macros, so that types get resolved correctly. For the definition of the trait, you can just write Option etc.

konsumlamm · 2021-08-04T01:18:57Z

We may also want to add the functions added in #244 as trait methods (which would also eliminate the need for the supertraits for ops). I was worried a bit about the names conflicting with the respective const fns (since trait methods can't be const currently, sadly), but there seem to be no problems.

KodrAus · 2021-08-04T01:39:51Z

I was worried a bit about the names conflicting with the respective const fns (since trait methods can't be const currently, sadly), but there seem to be no problems.

Ah yes, inherent methods are resolved over methods from traits or deref, so we'd be safe adding them to the trait.

We may also want to use some private supertrait to discourage callers from implementing this trait directly (we can't technically prevent it since expansion happens in their crates). That way we could mark the trait methods as const in the future when it's possible without needing a major version bump. Maybe something like:

#[doc(hidden)]
pub mod _private {
    pub trait ProducedByMacro {}
}

pub trait BitFlags: ProducedByMacro { .. }

konsumlamm · 2021-08-04T01:47:26Z

We may also want to use some private supertrait to discourage callers from implementing this trait directly (we can't technically prevent it since expansion happens in their crates). That way we could mark the trait methods as const in the future when it's possible without needing a major version bump. Maybe something like:
#[doc(hidden)]
pub mod _private {
    pub trait ProducedByMacro {}
}

pub trait BitFlags: ProducedByMacro { .. }

Why would this private supertrait have to be generated by a macro? It could just be defined as a normal trait (as is the case for the BitFlags trait in this PR). That way, we would have a truly sealed trait.

konsumlamm · 2021-08-04T11:13:44Z

src/bitflags_trait.rs

+    fn bits(&self) -> Self::Bits;
+    /// Convert from underlying bit representation, unless that
+    /// representation contains bits that do not correspond to a flag.
+    fn from_bits(bits: Self::Bits) -> _core::option::Option<Self>


Suggested change

fn from_bits(bits: Self::Bits) -> _core::option::Option<Self>

fn from_bits(bits: Self::Bits) -> Option<Self>

konsumlamm · 2021-08-04T11:15:34Z

src/bitflags_trait.rs

+
+pub trait BitFlags {


Suggested change

pub trait BitFlags {

/// A trait that is automatically implemented for all bitflags.

pub trait BitFlags {

KodrAus · 2021-08-04T23:06:22Z

Why would this private supertrait have to be generated by a macro? It could just be defined as a normal trait

Ah the pattern would be almost the same as the usual private::Sealed approach. I just pulled the name ProducedByMacro out to suggest to anyone looking at the docs that our requirement on implementing ProducedByMacro is that it came from the bitflags! macro.

The sealed trait (whether it's called ProducedByMacro, Sealed, or anything else) would live in bitflags, but unlike in the usual pattern we'd still need it to be publicly exported so that callers of bitflags! in external crates could still implement it along with BitFlags. We just don't want anyone implementing that sealed trait directly. And if they do they run the risk of breakage when we tweak BitFlags.

…lags_trait

arturoc · 2021-08-23T14:02:17Z

jjust found a moment to rebase on current main, every test seems to be passing now without problem

KodrAus · 2021-10-23T11:58:34Z

Thanks for working on this @arturoc! I'll go ahead and merge this in and send a follow-up that includes a few of the other things we were looking at.

arturoc · 2021-10-23T18:56:07Z

That's great! Btw, probably the generated example that serves as documentation is not necesary anymore since the trait documents everything already

matklad · 2021-10-25T09:50:01Z

src/lib.rs

+        impl $crate::BitFlags for $BitFlags {
+            type Bits = $T;
+
+            fn empty() -> Self {


You probably want to put inline on most methods here, to enable cross-crate inlining if called via a trait.

arturoc added 5 commits June 18, 2020 16:44

BitFlags trait

21931c1

Adds a BitFlags trait that is implemented by every bitflags. The trait includes documentation so it the current hack to document methods through an example could be removed Closes bitflags#154

missing trait bounds for BitFlags trait

cd6c42d

error output doesn't match current compiler

65632b9

fix trait bounds for older compilers

358d598

fix BitFlags trait for old compilers

7f77a4f

KodrAus reviewed May 16, 2021

View reviewed changes

KodrAus mentioned this pull request May 16, 2021

Implement Iterator for bitflags #204

Closed

KodrAus reviewed May 16, 2021

View reviewed changes

arturoc added 2 commits May 16, 2021 17:54

Merge branch 'master' of https://github.com/bitflags/bitflags into bi…

de1f39e

…tflags_trait

implement suggestions on PR

09e523e

konsumlamm reviewed Aug 4, 2021

View reviewed changes

Merge branch 'main' of https://github.com/bitflags/bitflags into bitf…

b08e99f

…lags_trait

KodrAus merged commit 2af5390 into bitflags:main Oct 23, 2021

This was referenced Oct 23, 2021

Add a hidden trait to discourage manual impls of BitFlags #261

Merged

Add an "impl_arbitrary" feature to enable implementing Arbitrary. #260

Closed

This was referenced Oct 23, 2021

From/TryFrom impls for generated types #224

Closed

Constants deserialization #147

Closed

Considerations for 2.0 #262

Closed

matklad reviewed Oct 25, 2021

View reviewed changes

KodrAus mentioned this pull request Mar 10, 2023

Prepare for 2.0.0 release #306

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BitFlags trait #220

BitFlags trait #220

arturoc commented Jun 18, 2020

Adam-Gleave commented Mar 13, 2021

KodrAus May 16, 2021

arturoc May 16, 2021

KodrAus May 16, 2021

KodrAus May 16, 2021

KodrAus commented May 16, 2021

KodrAus May 16, 2021

arturoc May 16, 2021

KodrAus May 16, 2021

arturoc commented May 17, 2021

bradleyharden commented Jun 25, 2021

KodrAus commented Aug 3, 2021

KodrAus commented Aug 3, 2021

konsumlamm Aug 4, 2021 •

edited

konsumlamm commented Aug 4, 2021

KodrAus commented Aug 4, 2021

konsumlamm commented Aug 4, 2021

konsumlamm Aug 4, 2021

konsumlamm Aug 4, 2021

KodrAus commented Aug 4, 2021

arturoc commented Aug 23, 2021

KodrAus commented Oct 23, 2021

arturoc commented Oct 23, 2021

matklad Oct 25, 2021

	fn from_bits(bits: Self::Bits) -> _core::option::Option<Self>
	fn from_bits(bits: Self::Bits) -> Option<Self>

BitFlags trait #220

BitFlags trait #220

Conversation

arturoc commented Jun 18, 2020

Adam-Gleave commented Mar 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KodrAus commented May 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturoc commented May 17, 2021

bradleyharden commented Jun 25, 2021

KodrAus commented Aug 3, 2021

KodrAus commented Aug 3, 2021

konsumlamm Aug 4, 2021 • edited

Choose a reason for hiding this comment

konsumlamm commented Aug 4, 2021

KodrAus commented Aug 4, 2021

konsumlamm commented Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KodrAus commented Aug 4, 2021

arturoc commented Aug 23, 2021

KodrAus commented Oct 23, 2021

arturoc commented Oct 23, 2021

Choose a reason for hiding this comment

konsumlamm Aug 4, 2021 •

edited