Message-ID: <CABVgOSkKD2Z60_+MoP_nu3YCUocvxJaohMBZpXy=_aavUZ99XA@mail.gmail.com>
Date: Tue, 18 Mar 2025 16:02:15 +0800
From: David Gow <davidgow@...gle.com>
To: Antonio Hickey <contact@...e-forge.io>
Cc: Brendan Higgins <brendan.higgins@...ux.dev>, Rae Moar <rmoar@...gle.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>, Antonio Hickey <contact@...oniohickey.com>, 
	linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 08/16] rust: kunit: refactor to use `&raw [const|mut]`

On Sun, 16 Mar 2025 at 14:20, Antonio Hickey <contact@...e-forge.io> wrote:
>
> Replacing all occurrences of `addr_of!(place)` and `addr_of_mut!(place)`
> with `&raw const place` and `&raw mut place` respectively.
>
> This will allow us to reduce macro complexity, and improve consistency
> with existing reference syntax as `&raw const`, `&raw mut` are similar
> to `&`, `&mut` making it fit more naturally with other existing code.
>
> Suggested-by: Benno Lossin <benno.lossin@...ton.me>
> Link: https://github.com/Rust-for-Linux/linux/issues/1148
> Signed-off-by: Antonio Hickey <contact@...oniohickey.com>
> ---

Thanks, Antonio.

So this looks fine, but it's also a bit annoying, as the remaining
KUnit/Rust integration patches[1] were recently updated to use
`addr_of_mut!()`, so either this patch, or those, will need updating.

In general, I think changes such as those in this series are going to
get progressively more prone to conflicts as Rust is adopted by other
subsystems, as the 'rust' tree won't be the only one carrying changes
which could be affected. Maybe in the future it'd make sense to split
these up into a series which enables the new feature, and only then
put the warnings in place in the next version?

In the KUnit case in particular, since the patches haven't actually
been merged yet, we have three options:
- Merge this into rust-next, and send out a new version of the KUnit
patches which depend on it, which then are also carried in rust-next,
or delayed (again) to 6.16. I suspect given how close to the merge
window we are, the delay is more likely.
- Merge the KUnit changes as-is (either into rust-next or
kselftest/kunit), and send out a new version of this series which also
updates it (probably in 6.16, but maybe sooner if everything goes via
rust-next).
- Merge both, and accept that there'll be some clippy errors until a
follow-up patch fixing them is sent and merged.

As someone with a vested interest in the KUnit changes, and at best a
mild academic interest in the addr_of_muit!() deprecation, my
preferences are with the latter two options. (I'd also, in general,
whinge that the frequency of new Rust versions / warnings is high
enough that it's taking a not insignificant portion of the limited
time I have working on Rust things to understand and deal with the
churn.)

Regardless, apart from the minor irritation of having to learn yet
another new syntax, I have no objections to this patch in particular.

Reviewed-by: David Gow <davidgow@...gle.com>

Thanks (and sorry for the grumpiness: don't take it personally),
-- David

[1]: https://lore.kernel.org/rust-for-linux/20250307090103.918788-1-davidgow@google.com/

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5281 bytes)

Powered by blists - more mailing lists