On Mon, Mar 17, 2025 at 04:53:18PM -0400, Tamir Duberstein wrote:
On Mon, Mar 17, 2025 at 4:46 PM Boqun Feng boqun.feng@gmail.com wrote:
On Mon, Mar 17, 2025 at 04:35:42PM -0400, Tamir Duberstein wrote:
On Mon, Mar 17, 2025 at 4:28 PM Boqun Feng boqun.feng@gmail.com wrote:
On Mon, Mar 17, 2025 at 03:05:45PM -0400, Tamir Duberstein wrote:
On Mon, Mar 17, 2025 at 2:50 PM Tamir Duberstein tamird@gmail.com wrote:
On Mon, Mar 17, 2025 at 2:17 PM Boqun Feng boqun.feng@gmail.com wrote: > > Then we should fix clippy or how we set msrv rather adding the stub. > @Miguel?
I filed https://github.com/rust-lang/rust-clippy/issues/14425.
I don't think we can wait for that to be fixed, though. Usually clippy is distributed with rustc via rustup, so even if this is eventually fixed, all versions between 1.84.0 and the fix will need this workaround until MSRV is >= 1.84.0.
We need to take one step back to evalute this "workaround".
First, expose_provenance() and with_exposed_provenance{,_mut}() API are clearly defined as equavilent to `as` operation [1]. Therefore, the changes in this patch doing the conversion with expose_provenance() and with_exposed_provenance{,_mut}() don't change anything related to provenance in practice.
I do agree we want to use the explicit provenance API, but I don't think we want to introduce some API that we know we will change them latter when we bump the rustc minimal version. So the question is: are these stubs what we want even though in the future our minimal rustc version stablizes provenance API? If not, then the cost of this patch cannot justify its benefits IMO.
Now let's also look into why we choose a msrv for clippy, I would guess it's because we need to support all the versions of rustc starting at 1.78 and we want clippy to report a problem based on 1.78 even though we're using a higher version of rustc. But for this particular case, we use a feature that has already been stablized in a higher version of rustc, which means the problem reported by clippy doesn't help us, nor does it provide better code. Frankly speaking, I think we have other ways to ensure the support of all rustc versions without a msrv for clippy. If I was to choose, I would simply drop the msrv. But maybe I'm missing something.
The point is tools should help us to write good and maintainable code, we shouldn't introduce complicated structure of code just because some tools fail to do its job.
Even if we globally disable this clippy lint, we still need stubs because exposed_provenance was added in 1.79.0. Did your suggestion address this? Perhaps I missed it.
No, I didn't.
That's a separate topic though, because I can see the argument that: because with_exposed_provenance() is a function rather than a method, it won't be very benefical to use ptr::with_exposed_provenance() instead of kernel::with_exposed_provenance(), therefor these stubs of exposed_provenance make sense to exist. But I don't think the same argument works for ptr::{with_,map_,}addr().
What about `pointer::expose_provenance`? It's a method that was added in 1.79.0.
We have a few options:
1) we can decide to use funtion-version of expose_provenance() (i.e. the stub), if we feel the symmetry with with_exposed_provenance() is a strong rationale. This also means we won't likely use pointer::expose_provenance() in the future. That is, although kernel doesn't have stable internal API, but in the foreseeable future, we decide to use funtion-version of expose_provenance().
2) we can introduce a PtrExt trait for <1.79
pub trait PtrExt<T> { fn expose_provenance(self) -> usize; }
and
impl<T> PtrExt<T> for *const T { ... }
and `PtrExt` in kernel::prelude.
(we need to #[allow(unstable_name_collisions)] to make that work)
We can also make with_exposed_provenance() use the same *Ext trick, and remove it when we bump the minimal rustc version.
Regards, Boqun
We can certainly disable the clippy lint rather than add stubs for `pointer::{with_,map_,}addr`, but it doesn't bring us to a solution where only free functions require stubs.