Message-Id: <88BAA5DC-6025-45E4-88E8-396113766D32@collabora.com>
Date: Fri, 2 Jan 2026 11:43:54 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Kari Argillander <kari.argillander@...il.com>
Cc: Filipe Xavier <felipeaggger@...il.com>,
 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>,
 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 1 Jan 2026, at 18:39, Kari Argillander <kari.argillander@...il.com> wrote:
> 
> to 1.1.2026 klo 22.23 Daniel Almeida (daniel.almeida@...labora.com) kirjoitti:
>>> On 1 Jan 2026, at 16:14, Kari Argillander <kari.argillander@...il.com> wrote:
>>> 
>>> On Thu, 1 Jan 2026 at 20:21, Filipe Xavier <felipeaggger@...il.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

Powered by blists - more mailing lists