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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> wrote: > > > > > > > > > > > > On Mon, Mar 17, 2025 at 2:17 PM Boqun Feng <boqun.feng@xxxxxxxxx> 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. > > > > > > > > [1]: https://doc.rust-lang.org/std/ptr/fn.with_exposed_provenance_mut.html > > > > > > 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.
