Re: [PATCH v7] rust: add new macro for common bitmap operations

[Miguel Ojeda]

On Thu, Jan 1, 2026 at 7:21 PM Filipe Xavier <felipeaggger@xxxxxxxxx> 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