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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CANiq72mqUwscPtsOmtFhOThxyChv4vBTKza6uwN9K9CU4Hbqjw@mail.gmail.com>
Date: Sat, 3 Jan 2026 15:24:52 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Filipe Xavier <felipeaggger@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, daniel.almeida@...labora.com, 
	rust-for-linux@...r.kernel.org, felipe_life@...e.com, 
	linux-kernel@...r.kernel.org, Lyude Paul <lyude@...hat.com>
Subject: Re: [PATCH v7] rust: add new macro for common bitmap operations

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