rust/kernel/impl_flags.rs | 250 ++++++++++++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 2 + rust/kernel/prelude.rs | 1 + 3 files changed, 253 insertions(+)
We have seen a proliferation of mod_whatever::foo::Flags
being defined with essentially the same implementation
for BitAnd, BitOr, contains and etc.
This macro aims to bring a solution for this,
allowing to generate these methods for user-defined structs.
With some use cases in KMS and upcoming GPU drivers.
Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/We.20really.20need.20a.20common.20.60Flags.60.20type
Signed-off-by: Filipe Xavier <felipeaggger@gmail.com>
Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
Suggested-by: Lyude Paul <lyude@redhat.com>
---
Changes in v7:
- Mask invalid bits: add all_bits mask to preserve only valid bits in Not and BitXor ops.
- Link to v6: https://lore.kernel.org/r/20251205-feat-add-bitmask-macro-v6-1-31fbbf14b15a@gmail.com
Changes in v6:
- New methods: add new methods contains_any and contains_all.
- Link to v5: https://lore.kernel.org/r/20251109-feat-add-bitmask-macro-v5-1-9d911b207ef4@gmail.com
Changes in v5:
- Docs: improve macro documentation.
- Link to v4: https://lore.kernel.org/r/20251026-feat-add-bitmask-macro-v4-1-e1b59b4762bc@gmail.com
Changes in v4:
- Use enum: changed flag type from struct to enum.
- Minor fix: airect casting (flag as $ty) instead of field access (.0).
- Link to v3: https://lore.kernel.org/r/20250411-feat-add-bitmask-macro-v3-1-187bd3e4a03e@gmail.com
Changes in v3:
- New Feat: added support to declare flags inside macro use.
- Minor fixes: used absolute paths to refer to items, fix rustdoc and fix example cases.
- Link to v2: https://lore.kernel.org/r/20250325-feat-add-bitmask-macro-v2-1-d3beabdad90f@gmail.com
Changes in v2:
- rename: change macro and file name to impl_flags.
- negation sign: change char for negation to `!`.
- transpose docs: add support to transpose user provided docs.
- visibility: add support to use user defined visibility.
- operations: add new operations for flag,
to support use between bit and bitmap, eg: flag & flags.
- code style: small fixes to remove warnings.
- Link to v1: https://lore.kernel.org/r/20250304-feat-add-bitmask-macro-v1-1-1c2d2bcb476b@gmail.com
---
rust/kernel/impl_flags.rs | 250 ++++++++++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
rust/kernel/prelude.rs | 1 +
3 files changed, 253 insertions(+)
diff --git a/rust/kernel/impl_flags.rs b/rust/kernel/impl_flags.rs
new file mode 100644
index 0000000000000000000000000000000000000000..e908142f9a474ed79a5168bad2bcfe2b2e06ee40
--- /dev/null
+++ b/rust/kernel/impl_flags.rs
@@ -0,0 +1,250 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/// Common helper for declaring bitflag and bitmask types.
+///
+/// This macro handles:
+/// - A struct representing a bitmask, and an enumerator representing bitflags which
+/// may be used in the aforementioned bitmask.
+/// - Implementations of common bitmap op. ([`::core::ops::BitOr`], [`::core::ops::BitAnd`], etc.).
+/// - Utility methods such as `.contains()` to check flags.
+///
+/// # Examples
+///
+/// Defining and using impl_flags:
+///
+/// ```
+/// use kernel::impl_flags;
+///
+/// impl_flags!(
+/// /// Represents multiple permissions.
+/// #[derive(Debug, Clone, Default, Copy, PartialEq, Eq)]
+/// pub struct Permissions(u32);
+/// /// Represents a single permission.
+/// #[derive(Debug, Clone, Copy, PartialEq, Eq)]
+/// pub enum Permission {
+/// Read = 1 << 0,
+/// Write = 1 << 1,
+/// Execute = 1 << 2,
+/// }
+/// );
+///
+/// // Combine multiple permissions using operation OR (`|`).
+/// let read_write: Permissions = Permission::Read | Permission::Write;
+///
+/// assert!(read_write.contains(Permission::Read));
+/// assert!(read_write.contains(Permission::Write));
+/// assert!(!read_write.contains(Permission::Execute));
+/// assert!(read_write.contains_any(Permission::Read | Permission::Execute));
+/// assert!(read_write.contains_all(Permission::Read | Permission::Write));
+///
+/// // Removing a permission with operation AND (`&`).
+/// let read_only: Permissions = read_write & Permission::Read;
+/// assert!(read_only.contains(Permission::Read));
+/// assert!(!read_only.contains(Permission::Write));
+///
+/// // Toggling permissions with XOR (`^`).
+/// let toggled: Permissions = read_only ^ Permission::Read;
+/// assert!(!toggled.contains(Permission::Read));
+///
+/// // Inverting permissions with negation (`!`).
+/// let negated = !read_only;
+/// assert!(negated.contains(Permission::Write));
+/// assert!(!negated.contains(Permission::Read));
+/// ```
+#[macro_export]
+macro_rules! impl_flags {
+ (
+ $(#[$outer_flags:meta])*
+ $vis_flags:vis struct $flags:ident($ty:ty);
+
+ $(#[$outer_flag:meta])*
+ $vis_flag:vis enum $flag:ident {
+ $(
+ $(#[$inner_flag:meta])*
+ $name:ident = $value:expr
+ ),+ $( , )?
+ }
+ ) => {
+ $(#[$outer_flags])*
+ #[repr(transparent)]
+ $vis_flags struct $flags($ty);
+
+ $(#[$outer_flag])*
+ #[repr($ty)]
+ $vis_flag enum $flag {
+ $(
+ $(#[$inner_flag])*
+ $name = $value
+ ),+
+ }
+
+ impl ::core::convert::From<$flag> for $flags {
+ #[inline]
+ fn from(value: $flag) -> Self {
+ Self(value as $ty)
+ }
+ }
+
+ impl ::core::convert::From<$flags> for $ty {
+ #[inline]
+ fn from(value: $flags) -> Self {
+ value.0
+ }
+ }
+
+ impl ::core::ops::BitOr for $flags {
+ type Output = Self;
+ #[inline]
+ fn bitor(self, rhs: Self) -> Self::Output {
+ Self(self.0 | rhs.0)
+ }
+ }
+
+ impl ::core::ops::BitOrAssign for $flags {
+ #[inline]
+ fn bitor_assign(&mut self, rhs: Self) {
+ *self = *self | rhs;
+ }
+ }
+
+ impl ::core::ops::BitAnd for $flags {
+ type Output = Self;
+ #[inline]
+ fn bitand(self, rhs: Self) -> Self::Output {
+ Self(self.0 & rhs.0)
+ }
+ }
+
+ impl ::core::ops::BitAndAssign for $flags {
+ #[inline]
+ fn bitand_assign(&mut self, rhs: Self) {
+ *self = *self & rhs;
+ }
+ }
+
+ impl ::core::ops::BitOr<$flag> for $flags {
+ type Output = Self;
+ #[inline]
+ fn bitor(self, rhs: $flag) -> Self::Output {
+ self | Self::from(rhs)
+ }
+ }
+
+ impl ::core::ops::BitOrAssign<$flag> for $flags {
+ #[inline]
+ fn bitor_assign(&mut self, rhs: $flag) {
+ *self = *self | rhs;
+ }
+ }
+
+ impl ::core::ops::BitAnd<$flag> for $flags {
+ type Output = Self;
+ #[inline]
+ fn bitand(self, rhs: $flag) -> Self::Output {
+ self & Self::from(rhs)
+ }
+ }
+
+ impl ::core::ops::BitAndAssign<$flag> for $flags {
+ #[inline]
+ fn bitand_assign(&mut self, rhs: $flag) {
+ *self = *self & rhs;
+ }
+ }
+
+ impl ::core::ops::BitXor for $flags {
+ type Output = Self;
+ #[inline]
+ fn bitxor(self, rhs: Self) -> Self::Output {
+ Self((self.0 ^ rhs.0) & Self::all_bits())
+ }
+ }
+
+ impl ::core::ops::BitXorAssign for $flags {
+ #[inline]
+ fn bitxor_assign(&mut self, rhs: Self) {
+ *self = *self ^ rhs;
+ }
+ }
+
+ impl ::core::ops::Not for $flags {
+ type Output = Self;
+ #[inline]
+ fn not(self) -> Self::Output {
+ Self((!self.0) & Self::all_bits())
+ }
+ }
+
+ impl ::core::ops::BitOr for $flag {
+ type Output = $flags;
+ #[inline]
+ fn bitor(self, rhs: Self) -> Self::Output {
+ $flags(self as $ty | rhs as $ty)
+ }
+ }
+
+ impl ::core::ops::BitAnd for $flag {
+ type Output = $flags;
+ #[inline]
+ fn bitand(self, rhs: Self) -> Self::Output {
+ $flags(self as $ty & rhs as $ty)
+ }
+ }
+
+ impl ::core::ops::BitXor for $flag {
+ type Output = $flags;
+ #[inline]
+ fn bitxor(self, rhs: Self) -> Self::Output {
+ $flags((self as $ty ^ rhs as $ty) & $flags::all_bits())
+ }
+ }
+
+ impl ::core::ops::Not for $flag {
+ type Output = $flags;
+ #[inline]
+ fn not(self) -> Self::Output {
+ $flags((!(self as $ty)) & $flags::all_bits())
+ }
+ }
+
+ impl ::core::ops::BitXor<$flag> for $flags {
+ type Output = Self;
+ #[inline]
+ fn bitxor(self, rhs: $flag) -> Self::Output {
+ self ^ Self::from(rhs)
+ }
+ }
+
+ impl $flags {
+ /// Returns an empty instance of `type` where no flags are set.
+ #[inline]
+ pub const fn empty() -> Self {
+ Self(0)
+ }
+
+ /// Returns a mask containing all valid flag bits.
+ #[inline]
+ pub const fn all_bits() -> $ty {
+ 0 $( | $value )+
+ }
+
+ /// Checks if a specific flag is set.
+ #[inline]
+ pub fn contains(self, flag: $flag) -> bool {
+ (self.0 & flag as $ty) == flag as $ty
+ }
+
+ /// Checks if at least one of the provided flags is set.
+ #[inline]
+ pub fn contains_any(self, flags: $flags) -> bool {
+ (self.0 & flags.0) != 0
+ }
+
+ /// Checks if all of the provided flags are set.
+ #[inline]
+ pub fn contains_all(self, flags: $flags) -> bool {
+ (self.0 & flags.0) == flags.0
+ }
+ }
+ };
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 28007be98fbad0e875d7e5345e164e2af2c5da32..57b789315deda3d2fa8961af79cad4462fff7fa8 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -67,6 +67,8 @@
#[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
pub mod firmware;
pub mod fs;
+#[doc(hidden)]
+pub mod impl_flags;
pub mod init;
pub mod io;
pub mod ioctl;
diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index baa774a351ceeb995a2a647f78a27b408d9f3834..085f23502544c80c2202eacd18ba190186a74f30 100644
--- a/rust/kernel/prelude.rs
+++ b/rust/kernel/prelude.rs
@@ -27,6 +27,7 @@
#[doc(no_inline)]
pub use super::dbg;
pub use super::fmt;
+pub use super::impl_flags;
pub use super::{dev_alert, dev_crit, dev_dbg, dev_emerg, dev_err, dev_info, dev_notice, dev_warn};
pub use super::{pr_alert, pr_crit, pr_debug, pr_emerg, pr_err, pr_info, pr_notice, pr_warn};
---
base-commit: edc5e6e019c99b529b3d1f2801d5cce9924ae79b
change-id: 20250304-feat-add-bitmask-macro-6424b1c317e2
Best regards,
--
Filipe Xavier <felipeaggger@gmail.com>On Thu, Jan 1, 2026 at 7:21 PM Filipe Xavier <felipeaggger@gmail.com> wrote:
>
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/// Common helper for declaring bitflag and bitmask types.
Please add a `//!` doc comment between these, even if it is a trivial
single line one, that describes what the module is about -- we have
them in modules even if they are `doc(hidden)` ones.
> +/// This macro handles:
I am not a native speaker, but "handles" can be confused for input.
The documentation also mixes a bit input and output, so it isn't clear
what it does.
I would instead try to explain what the inputs are and what the macro
implements in return, e.g.
/// This macro takes as input:
/// - A struct representing ...
/// - An enumeration representing ...
///
/// And generates:
/// - ...
Or something like that. The example is currently doing most of the
work at the moment.
> +/// - A struct representing a bitmask, and an enumerator representing bitflags which
enumeration?
> +/// may be used in the aforementioned bitmask.
Missing spaces at the beginning to match indentation.
> +/// - Implementations of common bitmap op. ([`::core::ops::BitOr`], [`::core::ops::BitAnd`], etc.).
Why is "bitmap" used here (and in the commit title)? This is intended
only for single-word bitflags, no?
Also, "operators" (please avoid contractions in docs).
> +/// Defining and using impl_flags:
Intra-doc link?
> +/// pub struct Permissions(u32);
> +/// /// Represents a single permission.
Newline in between?
> +/// Read = 1 << 0,
> +/// Write = 1 << 1,
> +/// Execute = 1 << 2,
I assume `rustfmt` (in a separate file, i.e. not within the docs
example) would keep that indentation because it is a macro, but please
double-check.
> +/// // Combine multiple permissions using operation OR (`|`).
Should we say "...using the bitwise OR (`|`)" or similar to match
better the usual Rust terms/docs?
I would also update the other cases below, even the negation case to
bitwise NOT for clarity and consistency (the XOR line you have doesn't
follow the pattern anyway).
> +/// let read_write: Permissions = Permission::Read | Permission::Write;
> +///
> +/// assert!(read_write.contains(Permission::Read));
You have a new line here but not in the other examples -- I would just
remove it.
Also, we should probably have one example of an `*Assign` case to show
they are implemented as well.
Thanks!
(By the way, and this in unrelated to Filipe, this thread has quite a
lot of nested levels of quotes -- could we please trim replies a bit
more? It would help reading in e.g. lore.kernel.org)
Cheers,
Miguel
On Thu, 01 Jan 2026 15:21:16 -0300
Filipe Xavier <felipeaggger@gmail.com> wrote:
> We have seen a proliferation of mod_whatever::foo::Flags
> being defined with essentially the same implementation
> for BitAnd, BitOr, contains and etc.
>
> This macro aims to bring a solution for this,
> allowing to generate these methods for user-defined structs.
> With some use cases in KMS and upcoming GPU drivers.
>
> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/We.20really.20need.20a.20common.20.60Flags.60.20type
> Signed-off-by: Filipe Xavier <felipeaggger@gmail.com>
> Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Suggested-by: Lyude Paul <lyude@redhat.com>
> ---
> Changes in v7:
> - Mask invalid bits: add all_bits mask to preserve only valid bits in Not and BitXor ops.
> - Link to v6: https://lore.kernel.org/r/20251205-feat-add-bitmask-macro-v6-1-31fbbf14b15a@gmail.com
>
> Changes in v6:
> - New methods: add new methods contains_any and contains_all.
> - Link to v5: https://lore.kernel.org/r/20251109-feat-add-bitmask-macro-v5-1-9d911b207ef4@gmail.com
>
> Changes in v5:
> - Docs: improve macro documentation.
> - Link to v4: https://lore.kernel.org/r/20251026-feat-add-bitmask-macro-v4-1-e1b59b4762bc@gmail.com
>
> Changes in v4:
> - Use enum: changed flag type from struct to enum.
> - Minor fix: airect casting (flag as $ty) instead of field access (.0).
> - Link to v3: https://lore.kernel.org/r/20250411-feat-add-bitmask-macro-v3-1-187bd3e4a03e@gmail.com
>
> Changes in v3:
> - New Feat: added support to declare flags inside macro use.
> - Minor fixes: used absolute paths to refer to items, fix rustdoc and fix example cases.
> - Link to v2: https://lore.kernel.org/r/20250325-feat-add-bitmask-macro-v2-1-d3beabdad90f@gmail.com
>
> Changes in v2:
> - rename: change macro and file name to impl_flags.
> - negation sign: change char for negation to `!`.
> - transpose docs: add support to transpose user provided docs.
> - visibility: add support to use user defined visibility.
> - operations: add new operations for flag,
> to support use between bit and bitmap, eg: flag & flags.
> - code style: small fixes to remove warnings.
> - Link to v1: https://lore.kernel.org/r/20250304-feat-add-bitmask-macro-v1-1-1c2d2bcb476b@gmail.com
> ---
> rust/kernel/impl_flags.rs | 250 ++++++++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 2 +
> rust/kernel/prelude.rs | 1 +
> 3 files changed, 253 insertions(+)
>
> diff --git a/rust/kernel/impl_flags.rs b/rust/kernel/impl_flags.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..e908142f9a474ed79a5168bad2bcfe2b2e06ee40
> --- /dev/null
> +++ b/rust/kernel/impl_flags.rs
> @@ -0,0 +1,250 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/// Common helper for declaring bitflag and bitmask types.
> +///
> +/// This macro handles:
> +/// - A struct representing a bitmask, and an enumerator representing bitflags which
> +/// may be used in the aforementioned bitmask.
> +/// - Implementations of common bitmap op. ([`::core::ops::BitOr`], [`::core::ops::BitAnd`], etc.).
> +/// - Utility methods such as `.contains()` to check flags.
> +///
> +/// # Examples
> +///
> +/// Defining and using impl_flags:
> +///
> +/// ```
> +/// use kernel::impl_flags;
> +///
> +/// impl_flags!(
> +/// /// Represents multiple permissions.
> +/// #[derive(Debug, Clone, Default, Copy, PartialEq, Eq)]
> +/// pub struct Permissions(u32);
> +/// /// Represents a single permission.
> +/// #[derive(Debug, Clone, Copy, PartialEq, Eq)]
> +/// pub enum Permission {
> +/// Read = 1 << 0,
> +/// Write = 1 << 1,
> +/// Execute = 1 << 2,
> +/// }
> +/// );
> +///
> +/// // Combine multiple permissions using operation OR (`|`).
> +/// let read_write: Permissions = Permission::Read | Permission::Write;
> +///
> +/// assert!(read_write.contains(Permission::Read));
> +/// assert!(read_write.contains(Permission::Write));
> +/// assert!(!read_write.contains(Permission::Execute));
> +/// assert!(read_write.contains_any(Permission::Read | Permission::Execute));
> +/// assert!(read_write.contains_all(Permission::Read | Permission::Write));
> +///
> +/// // Removing a permission with operation AND (`&`).
> +/// let read_only: Permissions = read_write & Permission::Read;
> +/// assert!(read_only.contains(Permission::Read));
> +/// assert!(!read_only.contains(Permission::Write));
This comment is confusing. It should say "Masking a permission" instead of
"removing a permission" as it's the flags not mentioned that are removed.
Best,
Gary
> +///
> +/// // Toggling permissions with XOR (`^`).
> +/// let toggled: Permissions = read_only ^ Permission::Read;
> +/// assert!(!toggled.contains(Permission::Read));
> +///
> +/// // Inverting permissions with negation (`!`).
> +/// let negated = !read_only;
> +/// assert!(negated.contains(Permission::Write));
> +/// assert!(!negated.contains(Permission::Read));
> +/// ```
Filipe, you need to pick up tags when sending new versions. If you’re using b4, you can just do “b4 trailers -u” You missed my r-b from v6. — Daniel
On Thu, 1 Jan 2026 at 20:21, Filipe Xavier <felipeaggger@gmail.com> wrote:
> +/// // Combine multiple permissions using operation OR (`|`).
> +/// let read_write: Permissions = Permission::Read | Permission::Write;
> +///
> +/// assert!(read_write.contains(Permission::Read));
> +/// assert!(read_write.contains(Permission::Write));
> +/// assert!(!read_write.contains(Permission::Execute));
> +/// assert!(read_write.contains_any(Permission::Read | Permission::Execute));
> +/// assert!(read_write.contains_all(Permission::Read | Permission::Write));
> +///
> +/// // Removing a permission with operation AND (`&`).
> +/// let read_only: Permissions = read_write & Permission::Read;
> +/// assert!(read_only.contains(Permission::Read));
> +/// assert!(!read_only.contains(Permission::Write));
> +///
> +/// // Toggling permissions with XOR (`^`).
> +/// let toggled: Permissions = read_only ^ Permission::Read;
> +/// assert!(!toggled.contains(Permission::Read));
> +///
> +/// // Inverting permissions with negation (`!`).
> +/// let negated = !read_only;
> +/// assert!(negated.contains(Permission::Write));
> +/// assert!(!negated.contains(Permission::Read));
I really like the OR (`|`) operator. Personally, I don’t like anything
else. When
a function or expression gets longer, something like let negated = !read_only;
already feels strange to read.
My suggestion is that everything else should be functions, so we would write:
```rust
let xxx = Permission::Read | Permission::Write;
xxx.remove(Permission::Write | Permission::Read);
xxx.toggle(Permission::Execute);
xxx.invert();
```
This way we avoid expressions like:
```rust
let a = !(!xxx | yyy ^ zzz);
```
At that point, no one understands what is really happening anymore. People start
wondering whether operator precedence or evaluation order matters, and may even
have to go read the source code to figure it out.
By only supporting OR (|) as an operator and expressing everything else as
explicit function calls, the code becomes much easier to read, review, and
maintain.
Argillander
> On 1 Jan 2026, at 16:14, Kari Argillander <kari.argillander@gmail.com> wrote: > > On Thu, 1 Jan 2026 at 20:21, Filipe Xavier <felipeaggger@gmail.com> wrote: > >> +/// // Combine multiple permissions using operation OR (`|`). >> +/// let read_write: Permissions = Permission::Read | Permission::Write; >> +/// >> +/// assert!(read_write.contains(Permission::Read)); >> +/// assert!(read_write.contains(Permission::Write)); >> +/// assert!(!read_write.contains(Permission::Execute)); >> +/// assert!(read_write.contains_any(Permission::Read | Permission::Execute)); >> +/// assert!(read_write.contains_all(Permission::Read | Permission::Write)); >> +/// >> +/// // Removing a permission with operation AND (`&`). >> +/// let read_only: Permissions = read_write & Permission::Read; >> +/// assert!(read_only.contains(Permission::Read)); >> +/// assert!(!read_only.contains(Permission::Write)); >> +/// >> +/// // Toggling permissions with XOR (`^`). >> +/// let toggled: Permissions = read_only ^ Permission::Read; >> +/// assert!(!toggled.contains(Permission::Read)); >> +/// >> +/// // Inverting permissions with negation (`!`). >> +/// let negated = !read_only; >> +/// assert!(negated.contains(Permission::Write)); >> +/// assert!(!negated.contains(Permission::Read)); > > I really like the OR (`|`) operator. Personally, I don’t like anything > else. When > a function or expression gets longer, something like let negated = !read_only; > already feels strange to read. > > My suggestion is that everything else should be functions, so we would write: > > ```rust > let xxx = Permission::Read | Permission::Write; > xxx.remove(Permission::Write | Permission::Read); > xxx.toggle(Permission::Execute); > xxx.invert(); > ``` > > This way we avoid expressions like: > > ```rust > let a = !(!xxx | yyy ^ zzz); > ``` > “ > At that point, no one understands what is really happening anymore. People start > wondering whether operator precedence or evaluation order matters, and may even > have to go read the source code to figure it out. I disagree. How would that look like given your suggestion? > > By only supporting OR (|) as an operator and expressing everything else as > explicit function calls, the code becomes much easier to read, review, and > maintain. > > Argillander How is A | B readable but not A & B or A ^ B?
to 1.1.2026 klo 22.23 Daniel Almeida (daniel.almeida@collabora.com) kirjoitti:
> > On 1 Jan 2026, at 16:14, Kari Argillander <kari.argillander@gmail.com> wrote:
> >
> > On Thu, 1 Jan 2026 at 20:21, Filipe Xavier <felipeaggger@gmail.com> wrote:
> >
> >> +/// // Combine multiple permissions using operation OR (`|`).
> >> +/// let read_write: Permissions = Permission::Read | Permission::Write;
> >> +///
> >> +/// assert!(read_write.contains(Permission::Read));
> >> +/// assert!(read_write.contains(Permission::Write));
> >> +/// assert!(!read_write.contains(Permission::Execute));
> >> +/// assert!(read_write.contains_any(Permission::Read | Permission::Execute));
> >> +/// assert!(read_write.contains_all(Permission::Read | Permission::Write));
> >> +///
> >> +/// // Removing a permission with operation AND (`&`).
> >> +/// let read_only: Permissions = read_write & Permission::Read;
> >> +/// assert!(read_only.contains(Permission::Read));
> >> +/// assert!(!read_only.contains(Permission::Write));
> >> +///
> >> +/// // Toggling permissions with XOR (`^`).
> >> +/// let toggled: Permissions = read_only ^ Permission::Read;
> >> +/// assert!(!toggled.contains(Permission::Read));
> >> +///
> >> +/// // Inverting permissions with negation (`!`).
> >> +/// let negated = !read_only;
> >> +/// assert!(negated.contains(Permission::Write));
> >> +/// assert!(!negated.contains(Permission::Read));
> >
> > I really like the OR (`|`) operator. Personally, I don’t like anything
> > else. When
> > a function or expression gets longer, something like let negated = !read_only;
> > already feels strange to read.
> >
> > My suggestion is that everything else should be functions, so we would write:
> >
> > ```rust
> > let xxx = Permission::Read | Permission::Write;
> > xxx.remove(Permission::Write | Permission::Read);
> > xxx.toggle(Permission::Execute);
> > xxx.invert();
> > ```
> >
> > This way we avoid expressions like:
> >
> > ```rust
> > let a = !(!xxx | yyy ^ zzz);
> > ```
> >
> “
> > At that point, no one understands what is really happening anymore. People start
> > wondering whether operator precedence or evaluation order matters, and may even
> > have to go read the source code to figure it out.
>
> I disagree. How would that look like given your suggestion?
>
> >
> > By only supporting OR (|) as an operator and expressing everything else as
> > explicit function calls, the code becomes much easier to read, review, and
> > maintain.
> >
> > Argillander
>
> How is A | B readable but not A & B or A ^ B?
Because that is just a way to express multiple / group of Flags and nothing
more. And thinking of this it could be even & operator. So we only allow
creating / enabling / disabling / toggling for a group of flags. Everything
still needs to be explicit. So we would end up with something like
```rust
Flags::new(A | B);
toggle(A | B);
disable(A | B); // Or remove() (I do not have opinion)
enable(A | B);
// I would even consider & operator here as then you read it quite nicely
// toggle (A and B)
toggle(A & B);
```
Most annoying thing with flags is that if you need to enable 8 flags then some
operator becomes a must. Still using those in everywhere is error prone and
confusing. So I totally agreed that we need some way to express group of flags
without always doing something like "t.enable(Flag::A).enable(Flag::B)"
If this way is used it is really nice that every function get's documentation.
If we only allow | or & there is very little chance that someone mix what those
means as it is always documented function call. Using operators too freely feels
very C++ in my opinion. Rust is very explicit language so let's not change that
but let's still make it usable for our cases.
If this way is used it is really nice that every function get's documentation.
If we only allow | or & there is very little chance that someone mix what those
means as it is always documented function call. Using operators too freely feels
very C++ in my opinion. Rust is very explicit language so let's not change that
but let's still make it usable for our cases.
Argillander
> On 1 Jan 2026, at 18:39, Kari Argillander <kari.argillander@gmail.com> wrote: > > to 1.1.2026 klo 22.23 Daniel Almeida (daniel.almeida@collabora.com) kirjoitti: >>> On 1 Jan 2026, at 16:14, Kari Argillander <kari.argillander@gmail.com> wrote: >>> >>> On Thu, 1 Jan 2026 at 20:21, Filipe Xavier <felipeaggger@gmail.com> wrote: >>> >>>> +/// // Combine multiple permissions using operation OR (`|`). >>>> +/// let read_write: Permissions = Permission::Read | Permission::Write; >>>> +/// >>>> +/// assert!(read_write.contains(Permission::Read)); >>>> +/// assert!(read_write.contains(Permission::Write)); >>>> +/// assert!(!read_write.contains(Permission::Execute)); >>>> +/// assert!(read_write.contains_any(Permission::Read | Permission::Execute)); >>>> +/// assert!(read_write.contains_all(Permission::Read | Permission::Write)); >>>> +/// >>>> +/// // Removing a permission with operation AND (`&`). >>>> +/// let read_only: Permissions = read_write & Permission::Read; >>>> +/// assert!(read_only.contains(Permission::Read)); >>>> +/// assert!(!read_only.contains(Permission::Write)); >>>> +/// >>>> +/// // Toggling permissions with XOR (`^`). >>>> +/// let toggled: Permissions = read_only ^ Permission::Read; >>>> +/// assert!(!toggled.contains(Permission::Read)); >>>> +/// >>>> +/// // Inverting permissions with negation (`!`). >>>> +/// let negated = !read_only; >>>> +/// assert!(negated.contains(Permission::Write)); >>>> +/// assert!(!negated.contains(Permission::Read)); >>> >>> I really like the OR (`|`) operator. Personally, I don’t like anything >>> else. When >>> a function or expression gets longer, something like let negated = !read_only; >>> already feels strange to read. >>> >>> My suggestion is that everything else should be functions, so we would write: >>> >>> ```rust >>> let xxx = Permission::Read | Permission::Write; >>> xxx.remove(Permission::Write | Permission::Read); >>> xxx.toggle(Permission::Execute); >>> xxx.invert(); >>> ``` >>> >>> This way we avoid expressions like: >>> >>> ```rust >>> let a = !(!xxx | yyy ^ zzz); >>> ``` >>> >> “ >>> At that point, no one understands what is really happening anymore. People start >>> wondering whether operator precedence or evaluation order matters, and may even >>> have to go read the source code to figure it out. Then don’t write convoluted expressions…it’s not something you can fix at the language level. What’s your take on the boolean operators, by the way? >> >> I disagree. How would that look like given your suggestion? >> >>> >>> By only supporting OR (|) as an operator and expressing everything else as >>> explicit function calls, the code becomes much easier to read, review, and >>> maintain. >>> >>> Argillander >> >> How is A | B readable but not A & B or A ^ B? > > Because that is just a way to express multiple / group of Flags and nothing > more. And thinking of this it could be even & operator. So we only allow So A | B is “just a way to express multiple flags” instead of the much more obvious A OR B? Same for A & B . > creating / enabling / disabling / toggling for a group of flags. Everything > still needs to be explicit. So we would end up with something like > > ```rust > Flags::new(A | B); This is _much_ more straightforward: +/// // Combine multiple permissions using operation OR (`|`). +/// let read_write: Permissions = Permission::Read | Permission::Write; > toggle(A | B); > disable(A | B); // Or remove() (I do not have opinion) > enable(A | B); Now we have to break extremely straightforward operations into multiple function calls. This is not ergonomic. > > // I would even consider & operator here as then you read it quite nicely > // toggle (A and B) > toggle(A & B); > ``` Sorry, I don’t buy this syntax at all. What’s the difference between A | B vs A & B in your proposal? > > Most annoying thing with flags is that if you need to enable 8 flags then some > operator becomes a must. Still using those in everywhere is error prone and So you’re proposing both operators and these functions? > confusing. So I totally agreed that we need some way to express group of flags > without always doing something like "t.enable(Flag::A).enable(Flag::B)" > > If this way is used it is really nice that every function get's documentation. We do *not* need documentation for “toggle()”, “invert()” and friends. > If we only allow | or & there is very little chance that someone mix what those > means as it is always documented function call. Using operators too freely feels No kernel developer is ever going to get confused by basic bitwise operations. > very C++ in my opinion. Rust is very explicit language so let's not change that How is C++ relevant here? Bitwise ops are used all over the place in C too. > but let's still make it usable for our cases. The language specifically allows us to build this API, so I see no problem with the current approach. Your implementation is, in my humble opinion, a downgrade w.r.t to what has been developed so far: it changes a syntax that everyone understands in order to solve a problem that doesn’t really exist. > > Argillander
On 2.1.2026 16.44 Daniel Almeida (daniel.almeida@collabora.com) wrote:
> > On 1 Jan 2026, at 18:39, Kari Argillander <kari.argillander@gmail.com> wrote:
> >
> > to 1.1.2026 klo 22.23 Daniel Almeida (daniel.almeida@collabora.com) kirjoitti:
> >>> On 1 Jan 2026, at 16:14, Kari Argillander <kari.argillander@gmail.com> wrote:
> >>>
> >>> On Thu, 1 Jan 2026 at 20:21, Filipe Xavier <felipeaggger@gmail.com> wrote:
> >>>
> >>>> +/// // Combine multiple permissions using operation OR (`|`).
> >>>> +/// let read_write: Permissions = Permission::Read | Permission::Write;
> >>>> +///
> >>>> +/// assert!(read_write.contains(Permission::Read));
> >>>> +/// assert!(read_write.contains(Permission::Write));
> >>>> +/// assert!(!read_write.contains(Permission::Execute));
> >>>> +/// assert!(read_write.contains_any(Permission::Read | Permission::Execute));
> >>>> +/// assert!(read_write.contains_all(Permission::Read | Permission::Write));
> >>>> +///
> >>>> +/// // Removing a permission with operation AND (`&`).
> >>>> +/// let read_only: Permissions = read_write & Permission::Read;
> >>>> +/// assert!(read_only.contains(Permission::Read));
> >>>> +/// assert!(!read_only.contains(Permission::Write));
> >>>> +///
> >>>> +/// // Toggling permissions with XOR (`^`).
> >>>> +/// let toggled: Permissions = read_only ^ Permission::Read;
> >>>> +/// assert!(!toggled.contains(Permission::Read));
> >>>> +///
> >>>> +/// // Inverting permissions with negation (`!`).
> >>>> +/// let negated = !read_only;
> >>>> +/// assert!(negated.contains(Permission::Write));
> >>>> +/// assert!(!negated.contains(Permission::Read));
> >>>
> >>> I really like the OR (`|`) operator. Personally, I don’t like anything
> >>> else. When
> >>> a function or expression gets longer, something like let negated = !read_only;
> >>> already feels strange to read.
> >>>
> >>> My suggestion is that everything else should be functions, so we would write:
> >>>
> >>> ```rust
> >>> let xxx = Permission::Read | Permission::Write;
> >>> xxx.remove(Permission::Write | Permission::Read);
> >>> xxx.toggle(Permission::Execute);
> >>> xxx.invert();
> >>> ```
> >>>
> >>> This way we avoid expressions like:
> >>>
> >>> ```rust
> >>> let a = !(!xxx | yyy ^ zzz);
> >>> ```
> >>>
> >> “
> >>> At that point, no one understands what is really happening anymore. People start
> >>> wondering whether operator precedence or evaluation order matters, and may even
> >>> have to go read the source code to figure it out.
>
>
> Then don’t write convoluted expressions…it’s not something you can
> fix at the language level.
I disagree. Rust already fix lot of problems at language level. This sounds
to me it is the same as "Then don't write memory bugs, it's not something
you can fix at the language level". And Rust still did it.
> What’s your take on the boolean operators, by the way?
They are very nice. I do not see how this is relevant.
> >> I disagree. How would that look like given your suggestion?
> >>
> >>>
> >>> By only supporting OR (|) as an operator and expressing everything else as
> >>> explicit function calls, the code becomes much easier to read, review, and
> >>> maintain.
> >>>
> >>> Argillander
> >>
> >> How is A | B readable but not A & B or A ^ B?
> >
> > Because that is just a way to express multiple / group of Flags and nothing
> > more. And thinking of this it could be even & operator. So we only allow
>
> So A | B is “just a way to express multiple flags” instead of the much
> more obvious A OR B? Same for A & B .
>
> > creating / enabling / disabling / toggling for a group of flags. Everything
> > still needs to be explicit. So we would end up with something like
> >
> > ```rust
> > Flags::new(A | B);
>
> This is _much_ more straightforward:
>
> +/// // Combine multiple permissions using operation OR (`|`).
> +/// let read_write: Permissions = Permission::Read | Permission::Write;
>
> > toggle(A | B);
> > disable(A | B); // Or remove() (I do not have opinion)
> > enable(A | B);
>
> Now we have to break extremely straightforward operations into multiple
> function calls. This is not ergonomic.
>
> >
> > // I would even consider & operator here as then you read it quite nicely
> > // toggle (A and B)
> > toggle(A & B);
> > ```
>
> Sorry, I don’t buy this syntax at all. What’s the difference between A | B vs
> A & B in your proposal?
Following is a problem in my opinion. Rust use | for OR in match statements.
So if we do bitflag operations it looks quite strange when we do this:
```
const WRITE_READ: Permissions = Permission::Write | Permission::Read;
match permissions {
Permissions::None => println!("None"),
Permissions::All => println!("All"),
Permission::Write | Permission::Read => println!("Write or read"),
WRITE_READ=> println!("Write and read"),
_ => {},
}
```
This has actually been documented in rust bitflags [1]. I also think we should
document this somewhere because this is not very obvious in Rust.
[1]: https://docs.rs/bitflags/latest/bitflags/macro.bitflags_match.html
> >
> > Most annoying thing with flags is that if you need to enable 8 flags then some
> > operator becomes a must. Still using those in everywhere is error prone and
>
> So you’re proposing both operators and these functions?
>
> > confusing. So I totally agreed that we need some way to express group of flags
> > without always doing something like "t.enable(Flag::A).enable(Flag::B)"
> >
> > If this way is used it is really nice that every function get's documentation.
>
> We do *not* need documentation for “toggle()”, “invert()” and friends.
>
>
> > If we only allow | or & there is very little chance that someone mix what those
> > means as it is always documented function call. Using operators too freely feels
>
> No kernel developer is ever going to get confused by basic bitwise operations.
Most developers just need to write drivers.
> > very C++ in my opinion. Rust is very explicit language so let's not change that
>
> How is C++ relevant here? Bitwise ops are used all over the place in C too.
Sorry for my bad words here. I meant C++ operator overloads, not
bitwise operations
themself. Those are used very freely in many places So you actually do
not know what
even "+ operators" do anymore.
> > but let's still make it usable for our cases.
>
> The language specifically allows us to build this API, so I see no problem with
> the current approach. Your implementation is, in my humble opinion, a downgrade
> w.r.t to what has been developed so far: it changes a syntax that everyone
> understands in order to solve a problem that doesn’t really exist.
bitwise operations are backed in C. In Rust you really have to know
that now this
struct supports bitwise operations. That is not so obvious in Rust
that you can do
those operations for non-integer types.
But probably there is no point continue discussion as two people has nacked
my suggestion so fast and I'm totally ok with it. Still thanks for
taking time on this.
Argillander
On Sat, Jan 3, 2026 at 2:15 AM Kari Argillander <kari.argillander@gmail.com> wrote: > > But probably there is no point continue discussion as two people has nacked > my suggestion so fast and I'm totally ok with it. Still thanks for > taking time on this. In general, I think it is a good point, i.e. I agree that abusing custom operators everywhere isn't great, just like having a lot of macros with custom, non-obvious syntax isn't ideal. For something like bitflags, however, it seems reasonable. I think it is a balance. For instance, `.enable()` and `.toggle()` are common in other domains especially with a single flag/concept as an argument, but when working with bitflags developers will likely expect to be able to use the usual expressions they are accustomed to, especially in a context like the Linux kernel. And while the `|` approach would be workable if you introduce more methods like the masking case etc., something else like `.toggle(A & B)` would be very, very confusing, i.e. it is like going back to square one introducing custom syntax again. Thanks for raising the point -- it is always nice to consider more viewpoints and having more reviews. Cheers, Miguel
On Thu, 1 Jan 2026 23:39:26 +0200 Kari Argillander <kari.argillander@gmail.com> wrote: > to 1.1.2026 klo 22.23 Daniel Almeida (daniel.almeida@collabora.com) kirjoitti: > > > On 1 Jan 2026, at 16:14, Kari Argillander <kari.argillander@gmail.com> wrote: > > > > > > On Thu, 1 Jan 2026 at 20:21, Filipe Xavier <felipeaggger@gmail.com> wrote: > > > > > >> +/// // Combine multiple permissions using operation OR (`|`). > > >> +/// let read_write: Permissions = Permission::Read | Permission::Write; > > >> +/// > > >> +/// assert!(read_write.contains(Permission::Read)); > > >> +/// assert!(read_write.contains(Permission::Write)); > > >> +/// assert!(!read_write.contains(Permission::Execute)); > > >> +/// assert!(read_write.contains_any(Permission::Read | Permission::Execute)); > > >> +/// assert!(read_write.contains_all(Permission::Read | Permission::Write)); > > >> +/// > > >> +/// // Removing a permission with operation AND (`&`). > > >> +/// let read_only: Permissions = read_write & Permission::Read; > > >> +/// assert!(read_only.contains(Permission::Read)); > > >> +/// assert!(!read_only.contains(Permission::Write)); > > >> +/// > > >> +/// // Toggling permissions with XOR (`^`). > > >> +/// let toggled: Permissions = read_only ^ Permission::Read; > > >> +/// assert!(!toggled.contains(Permission::Read)); > > >> +/// > > >> +/// // Inverting permissions with negation (`!`). > > >> +/// let negated = !read_only; > > >> +/// assert!(negated.contains(Permission::Write)); > > >> +/// assert!(!negated.contains(Permission::Read)); > > > > > > I really like the OR (`|`) operator. Personally, I don’t like anything > > > else. When > > > a function or expression gets longer, something like let negated = !read_only; > > > already feels strange to read. > > > > > > My suggestion is that everything else should be functions, so we would write: > > > > > > ```rust > > > let xxx = Permission::Read | Permission::Write; > > > xxx.remove(Permission::Write | Permission::Read); > > > xxx.toggle(Permission::Execute); > > > xxx.invert(); > > > ``` > > > > > > This way we avoid expressions like: > > > > > > ```rust > > > let a = !(!xxx | yyy ^ zzz); > > > ``` > > > > > “ > > > At that point, no one understands what is really happening anymore. People start > > > wondering whether operator precedence or evaluation order matters, and may even > > > have to go read the source code to figure it out. > > > > I disagree. How would that look like given your suggestion? > > > > > > > > By only supporting OR (|) as an operator and expressing everything else as > > > explicit function calls, the code becomes much easier to read, review, and > > > maintain. > > > > > > Argillander > > > > How is A | B readable but not A & B or A ^ B? > > Because that is just a way to express multiple / group of Flags and nothing > more. And thinking of this it could be even & operator. So we only allow > creating / enabling / disabling / toggling for a group of flags. Everything > still needs to be explicit. So we would end up with something like > > ```rust > Flags::new(A | B); > toggle(A | B); > disable(A | B); // Or remove() (I do not have opinion) > enable(A | B); > > // I would even consider & operator here as then you read it quite nicely > // toggle (A and B) > toggle(A & B); > ``` > > Most annoying thing with flags is that if you need to enable 8 flags then some > operator becomes a must. Still using those in everywhere is error prone and > confusing. So I totally agreed that we need some way to express group of flags > without always doing something like "t.enable(Flag::A).enable(Flag::B)" > > If this way is used it is really nice that every function get's documentation. > If we only allow | or & there is very little chance that someone mix what those > means as it is always documented function call. Using operators too freely feels > very C++ in my opinion. Rust is very explicit language so let's not change that > but let's still make it usable for our cases. I don't see why using operators is C++ specific. You do use these operators in C, too. I would rather write `flag & mask` rather than `flag.remove(mask.invert())`. Using operators make the bitflags nature explicit to the users and I think that's a pro not a con. Best, Gary > > If this way is used it is really nice that every function get's documentation. > If we only allow | or & there is very little chance that someone mix what those > means as it is always documented function call. Using operators too freely feels > very C++ in my opinion. Rust is very explicit language so let's not change that > but let's still make it usable for our cases. > > Argillander
to 1.1.2026 klo 23.39 Kari Argillander (kari.argillander@gmail.com) kirjoitti:
>
> to 1.1.2026 klo 22.23 Daniel Almeida (daniel.almeida@collabora.com) kirjoitti:
> > > On 1 Jan 2026, at 16:14, Kari Argillander <kari.argillander@gmail.com> wrote:
> > >
> > > On Thu, 1 Jan 2026 at 20:21, Filipe Xavier <felipeaggger@gmail.com> wrote:
> > >
> > >> +/// // Combine multiple permissions using operation OR (`|`).
> > >> +/// let read_write: Permissions = Permission::Read | Permission::Write;
> > >> +///
> > >> +/// assert!(read_write.contains(Permission::Read));
> > >> +/// assert!(read_write.contains(Permission::Write));
> > >> +/// assert!(!read_write.contains(Permission::Execute));
> > >> +/// assert!(read_write.contains_any(Permission::Read | Permission::Execute));
> > >> +/// assert!(read_write.contains_all(Permission::Read | Permission::Write));
> > >> +///
> > >> +/// // Removing a permission with operation AND (`&`).
> > >> +/// let read_only: Permissions = read_write & Permission::Read;
> > >> +/// assert!(read_only.contains(Permission::Read));
> > >> +/// assert!(!read_only.contains(Permission::Write));
> > >> +///
> > >> +/// // Toggling permissions with XOR (`^`).
> > >> +/// let toggled: Permissions = read_only ^ Permission::Read;
> > >> +/// assert!(!toggled.contains(Permission::Read));
> > >> +///
> > >> +/// // Inverting permissions with negation (`!`).
> > >> +/// let negated = !read_only;
> > >> +/// assert!(negated.contains(Permission::Write));
> > >> +/// assert!(!negated.contains(Permission::Read));
> > >
> > > I really like the OR (`|`) operator. Personally, I don’t like anything
> > > else. When
> > > a function or expression gets longer, something like let negated = !read_only;
> > > already feels strange to read.
> > >
> > > My suggestion is that everything else should be functions, so we would write:
> > >
> > > ```rust
> > > let xxx = Permission::Read | Permission::Write;
> > > xxx.remove(Permission::Write | Permission::Read);
> > > xxx.toggle(Permission::Execute);
> > > xxx.invert();
> > > ```
> > >
> > > This way we avoid expressions like:
> > >
> > > ```rust
> > > let a = !(!xxx | yyy ^ zzz);
> > > ```
> > >
> > “
> > > At that point, no one understands what is really happening anymore. People start
> > > wondering whether operator precedence or evaluation order matters, and may even
> > > have to go read the source code to figure it out.
> >
> > I disagree. How would that look like given your suggestion?
> >
> > >
> > > By only supporting OR (|) as an operator and expressing everything else as
> > > explicit function calls, the code becomes much easier to read, review, and
> > > maintain.
> > >
> > > Argillander
> >
> > How is A | B readable but not A & B or A ^ B?
>
> Because that is just a way to express multiple / group of Flags and nothing
> more. And thinking of this it could be even & operator. So we only allow
> creating / enabling / disabling / toggling for a group of flags. Everything
> still needs to be explicit. So we would end up with something like
>
> ```rust
> Flags::new(A | B);
> toggle(A | B);
> disable(A | B); // Or remove() (I do not have opinion)
> enable(A | B);
>
> // I would even consider & operator here as then you read it quite nicely
> // toggle (A and B)
> toggle(A & B);
> ```
I tried implementing and my suggestion is something like this:
```rust
let mut f = Flags::new(Flag::Read & Flag::Write);
f.enable(Flag::Write & Flag::Execute)
.toggle(Flag::Write & Flag::Execute)
.invert()
.invert();
// ALL-of-these (built with &)
assert!(!f.contains(Flag::Write & Flag::Execute));
// ANY-of-these (built with |)
// Could also be contains_any() (Personally not fan).
// Or we can just do f.contains(Flag::Read) || f.contains(Flag::Execute)
// But this is quite nice imo
assert!(f.contains(Flag::Read | Flag::Execute));
// Or single flag
assert!(f.contains(Flag::Read));
// By design these won't build as does not make sense.
// let f = Flags::new(Flag::Read | Flag::Write);
// f.enable(Flag::Read | Flag::Write);
// f.toggle(Flag::Read | Flag::Write);
```
and you can try it yourself if wanted in [1]. I only played with the interface
and this is not ready for the kernel by any means.
[1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=b90f6e6e92b02ed6b90cbf7e094e5d48
© 2016 - 2026 Red Hat, Inc.