Rename the Delta struct's methods as_micros_ceil() and as_millis() to
into_micros_ceil() and into_millis() respectively, for consistency
with the naming of other methods.

Fix the commit 2ed94606a0fe ("rust: time: Rename Delta's methods from
as_* to into_*"), wasn't applied as expected, due to the conflict with
the commit 1b7bbd597527 ("rust: time: Avoid 64-bit integer division on
32-bit architectures").

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index eaa6d9ab5737..445877039395 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -284,7 +284,7 @@ pub const fn into_nanos(self) -> i64 {
     /// Return the smallest number of microseconds greater than or equal
     /// to the value in the [`Delta`].
     #[inline]
-    pub fn as_micros_ceil(self) -> i64 {
+    pub fn into_micros_ceil(self) -> i64 {
         #[cfg(CONFIG_64BIT)]
         {
             self.into_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
@@ -299,7 +299,7 @@ pub fn as_micros_ceil(self) -> i64 {
 
     /// Return the number of milliseconds in the [`Delta`].
     #[inline]
-    pub fn as_millis(self) -> i64 {
+    pub fn into_millis(self) -> i64 {
         #[cfg(CONFIG_64BIT)]
         {
             self.into_nanos() / NSEC_PER_MSEC
-- 
2.43.0

Hi Fujita,

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> Rename the Delta struct's methods as_micros_ceil() and as_millis() to
> into_micros_ceil() and into_millis() respectively, for consistency
> with the naming of other methods.
>
> Fix the commit 2ed94606a0fe ("rust: time: Rename Delta's methods from
> as_* to into_*"), wasn't applied as expected, due to the conflict with
> the commit 1b7bbd597527 ("rust: time: Avoid 64-bit integer division on
> 32-bit architectures").
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Sorry for messing up your patch. Since we have no rules against rebasing
rust-timekeeping and the PR is a few weeks down the road, I will just go
back and fix the original patch. Keep commit history clean.

Best regards,
Andreas Hindborg

On Thu, 19 Jun 2025 11:12:00 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

>> Rename the Delta struct's methods as_micros_ceil() and as_millis() to
>> into_micros_ceil() and into_millis() respectively, for consistency
>> with the naming of other methods.
>>
>> Fix the commit 2ed94606a0fe ("rust: time: Rename Delta's methods from
>> as_* to into_*"), wasn't applied as expected, due to the conflict with
>> the commit 1b7bbd597527 ("rust: time: Avoid 64-bit integer division on
>> 32-bit architectures").
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Sorry for messing up your patch. Since we have no rules against rebasing
> rust-timekeeping and the PR is a few weeks down the road, I will just go
> back and fix the original patch. Keep commit history clean.

I think it would be easier to resolve conflicts by applying the typed
clock sources patchset and the patchset to convert hrtimer first, and
then applying the division patch.

Then please ignore this and apply only the second patch in this
patchset.

Let me know if you need any help.


Thanks!

On Tue, Jun 17, 2025 at 11:41:54PM +0900, FUJITA Tomonori wrote:
> Rename the Delta struct's methods as_micros_ceil() and as_millis() to
> into_micros_ceil() and into_millis() respectively, for consistency
> with the naming of other methods.
> 
> Fix the commit 2ed94606a0fe ("rust: time: Rename Delta's methods from
> as_* to into_*"), wasn't applied as expected, due to the conflict with
> the commit 1b7bbd597527 ("rust: time: Avoid 64-bit integer division on
> 32-bit architectures").
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Why are we renaming them? The stdlib always uses as_* or to_* for copy
types. In my mind, into_* means that you want to emphasize that you are
performing a transformation that consumes self and transfers ownership
of some resource in the process.

See the api guidelines:
https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

Alice

On Wed, Jun 18, 2025 at 10:05 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> Why are we renaming them? The stdlib always uses as_* or to_* for copy
> types. In my mind, into_* means that you want to emphasize that you are
> performing a transformation that consumes self and transfers ownership
> of some resource in the process.
>
> See the api guidelines:
> https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

We may be going in circles here... I think the confusion is all on
what to do for "owned -> owned" `Copy` non-expensive conversions.

I think Tomo sent a patch to change the `as_` and `is_` methods to
take `&self` to be consistent with the guidelines, since they say
there is no "owned -> owned" case for `as_`. Then you mentioned that
`self` is OK and Andreas agreed, and I guess Tomo ended up with
`into_` since `to_` is only for the expensive case, even though it is
not meant for `Copy` types.

In other words, either we say in the kernel we are OK with `as_` for
"owned -> owned" too, or we take `&self`.

Did I get that right, everyone?

Thanks!

Cheers,
Miguel

On Wed, Jun 18, 2025 at 11:29:26AM +0200, Miguel Ojeda wrote:
> On Wed, Jun 18, 2025 at 10:05 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > Why are we renaming them? The stdlib always uses as_* or to_* for copy
> > types. In my mind, into_* means that you want to emphasize that you are
> > performing a transformation that consumes self and transfers ownership
> > of some resource in the process.
> >
> > See the api guidelines:
> > https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv
> 
> We may be going in circles here... I think the confusion is all on
> what to do for "owned -> owned" `Copy` non-expensive conversions.
> 
> I think Tomo sent a patch to change the `as_` and `is_` methods to
> take `&self` to be consistent with the guidelines, since they say
> there is no "owned -> owned" case for `as_`. Then you mentioned that
> `self` is OK and Andreas agreed, and I guess Tomo ended up with
> `into_` since `to_` is only for the expensive case, even though it is
> not meant for `Copy` types.
> 
> In other words, either we say in the kernel we are OK with `as_` for
> "owned -> owned" too, or we take `&self`.
> 
> Did I get that right, everyone?

Yeah I think using as_* naming for cases other than borrowed->borrowed
is relatively common for Copy types. Looking at the stdlib, the rules
I'm seeing are more or less these:

First, if the method is expensive the naming is to_* or a verb without a
prefix. We only use into_* for expensive operations if the call also
transfers ownership of some resource. Example: CStr::to_bytes()

Otherwise, if the method is something that looks inside the type (i.e.
it peels away a layer of abstraction), then we using as_* naming. Or we
might use a noun with no prefix if we want it to feel like a field
access. Example: Duration::as_millis()

On the other hand, if the method transforms the value while staying at
the same layer of abstraction, then we may call the method to_* (or just
use a verb without a prefix). Example: f64::to_radians()

Alice

On Wed, Jun 18, 2025 at 11:52 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> Yeah I think using as_* naming for cases other than borrowed->borrowed
> is relatively common for Copy types.

Hmm... from a quick look, I only see a few: on raw pointers, `NonNull`
and `Component`. But maybe I am grepping wrong.

There are a bunch on unstable and private stuff, e.g.
`ptr::Alignment`, so it may become a bit more common over time.

So far it seems most of them take `&self`, which seems to align with
the guidelines.

Either way, I agree that what is important is that it is a "free"
conversion/access/...

Cheers,
Miguel

On Wed, Jun 18, 2025 at 1:03 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Wed, Jun 18, 2025 at 11:52 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > Yeah I think using as_* naming for cases other than borrowed->borrowed
> > is relatively common for Copy types.
>
> Hmm... from a quick look, I only see a few: on raw pointers, `NonNull`
> and `Component`. But maybe I am grepping wrong.
>
> There are a bunch on unstable and private stuff, e.g.
> `ptr::Alignment`, so it may become a bit more common over time.
>
> So far it seems most of them take `&self`, which seems to align with
> the guidelines.
>
> Either way, I agree that what is important is that it is a "free"
> conversion/access/...

There are also methods such as Duration::as_millis(). Yes, those take
&self but &self is equivalent to self for Copy types, so there is no
difference. And even if we did treat them differently,
Duration::as_millis() is actually borrowed->owned as the return type
is not a reference, so ...

Alice

On Wed, Jun 18, 2025 at 3:17 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> There are also methods such as Duration::as_millis(). Yes, those take
> &self but &self is equivalent to self for Copy types, so there is no
> difference. And even if we did treat them differently,
> Duration::as_millis() is actually borrowed->owned as the return type
> is not a reference, so ...

In most cases it may not matter, but even if taking either was exactly
the same, the point of the discussion(s) was what is more idiomatic,
i.e. how to spell those signatures.

I understand you are saying that `Duration::as_millis()` is already a
stable example from the standard library of something that is not
borrowed -> borrowed, and thus the guidelines should be understood as
implying it is fine either way. It is still confusing, as shown by
these repeated discussions, and on the parameter's side of things,
they still seem to prefer `&self`, including in the equivalent methods
of this patch.

Personally, I would use `self`, and clarify the guidelines.

Cheers,
Miguel

On Wed, 18 Jun 2025 17:47:27 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Wed, Jun 18, 2025 at 3:17 PM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> There are also methods such as Duration::as_millis(). Yes, those take
>> &self but &self is equivalent to self for Copy types, so there is no
>> difference. And even if we did treat them differently,
>> Duration::as_millis() is actually borrowed->owned as the return type
>> is not a reference, so ...
> 
> In most cases it may not matter, but even if taking either was exactly
> the same, the point of the discussion(s) was what is more idiomatic,
> i.e. how to spell those signatures.
> 
> I understand you are saying that `Duration::as_millis()` is already a
> stable example from the standard library of something that is not
> borrowed -> borrowed, and thus the guidelines should be understood as
> implying it is fine either way. It is still confusing, as shown by
> these repeated discussions, and on the parameter's side of things,
> they still seem to prefer `&self`, including in the equivalent methods
> of this patch.
> 
> Personally, I would use `self`, and clarify the guidelines.

So would the function be defined like this?

fn as_nanos(self) -> i64;

If that's the case, then we've come full circle back to the original
problem; Clippy warns against using as_* names for trait methods that
take self as follows:

warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
   --> /home/fujita/git/linux-rust/rust/kernel/time/hrtimer.rs:430:17
    |
430 |     fn as_nanos(self) -> i64;
    |                 ^^^^
    |
    = help: consider choosing a less ambiguous name
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
    = note: `-W clippy::wrong-self-convention` implied by `-W clippy::all`
    = help: to override `-W clippy::all` add `#[allow(clippy::wrong_self_convention)]`

https://lore.kernel.org/rust-for-linux/20250610132823.3457263-2-fujita.tomonori@gmail.com/


HrTimerExpires trait needs as_nanos() method and Instant and Delta
need to implement HrTimerExpires trait.

We need a consistent definition of as_nanos() across the
HrTimerExpires trait, and the Instant and Delta structures.

And it would be better if the definition of as_nanos were consistent
with the other as_* methods.

It looks like the conversion from Delta to i64 doesn’t quite fit any
of the categories in the API guidelines. How should it be defined?

On Thu, Jun 19, 2025 at 8:08 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Wed, 18 Jun 2025 17:47:27 +0200
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>
> > On Wed, Jun 18, 2025 at 3:17 PM Alice Ryhl <aliceryhl@google.com> wrote:
> >>
> >> There are also methods such as Duration::as_millis(). Yes, those take
> >> &self but &self is equivalent to self for Copy types, so there is no
> >> difference. And even if we did treat them differently,
> >> Duration::as_millis() is actually borrowed->owned as the return type
> >> is not a reference, so ...
> >
> > In most cases it may not matter, but even if taking either was exactly
> > the same, the point of the discussion(s) was what is more idiomatic,
> > i.e. how to spell those signatures.
> >
> > I understand you are saying that `Duration::as_millis()` is already a
> > stable example from the standard library of something that is not
> > borrowed -> borrowed, and thus the guidelines should be understood as
> > implying it is fine either way. It is still confusing, as shown by
> > these repeated discussions, and on the parameter's side of things,
> > they still seem to prefer `&self`, including in the equivalent methods
> > of this patch.
> >
> > Personally, I would use `self`, and clarify the guidelines.
>
> So would the function be defined like this?
>
> fn as_nanos(self) -> i64;
>
> If that's the case, then we've come full circle back to the original
> problem; Clippy warns against using as_* names for trait methods that
> take self as follows:
>
> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
>    --> /home/fujita/git/linux-rust/rust/kernel/time/hrtimer.rs:430:17
>     |
> 430 |     fn as_nanos(self) -> i64;
>     |                 ^^^^
>     |
>     = help: consider choosing a less ambiguous name
>     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
>     = note: `-W clippy::wrong-self-convention` implied by `-W clippy::all`
>     = help: to override `-W clippy::all` add `#[allow(clippy::wrong_self_convention)]`
>
> https://lore.kernel.org/rust-for-linux/20250610132823.3457263-2-fujita.tomonori@gmail.com/

Are we missing a derive(Copy) for this type? Clippy does not emit that
lint if the type is Copy:
https://github.com/rust-lang/rust-clippy/issues/273

Alice

On Tue, 24 Jun 2025 13:15:32 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> So would the function be defined like this?
>>
>> fn as_nanos(self) -> i64;
>>
>> If that's the case, then we've come full circle back to the original
>> problem; Clippy warns against using as_* names for trait methods that
>> take self as follows:
>>
>> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
>>    --> /home/fujita/git/linux-rust/rust/kernel/time/hrtimer.rs:430:17
>>     |
>> 430 |     fn as_nanos(self) -> i64;
>>     |                 ^^^^
>>     |
>>     = help: consider choosing a less ambiguous name
>>     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
>>     = note: `-W clippy::wrong-self-convention` implied by `-W clippy::all`
>>     = help: to override `-W clippy::all` add `#[allow(clippy::wrong_self_convention)]`
>>
>> https://lore.kernel.org/rust-for-linux/20250610132823.3457263-2-fujita.tomonori@gmail.com/
> 
> Are we missing a derive(Copy) for this type? Clippy does not emit that
> lint if the type is Copy:
> https://github.com/rust-lang/rust-clippy/issues/273

I think that both Delta and Instant structs implement Copy.

#[repr(transparent)]
#[derive(PartialEq, PartialOrd, Eq, Ord)]
pub struct Instant<C: ClockSource> {
    inner: bindings::ktime_t,
    _c: PhantomData<C>,
}

impl<C: ClockSource> Clone for Instant<C> {
    fn clone(&self) -> Self {
        *self
    }
}

impl<C: ClockSource> Copy for Instant<C> {}

#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
pub struct Delta {
    nanos: i64,
}


The above warning is about the trait method.

On Tue, Jun 24, 2025 at 2:49 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Tue, 24 Jun 2025 13:15:32 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> >> So would the function be defined like this?
> >>
> >> fn as_nanos(self) -> i64;
> >>
> >> If that's the case, then we've come full circle back to the original
> >> problem; Clippy warns against using as_* names for trait methods that
> >> take self as follows:
> >>
> >> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
> >>    --> /home/fujita/git/linux-rust/rust/kernel/time/hrtimer.rs:430:17
> >>     |
> >> 430 |     fn as_nanos(self) -> i64;
> >>     |                 ^^^^
> >>     |
> >>     = help: consider choosing a less ambiguous name
> >>     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
> >>     = note: `-W clippy::wrong-self-convention` implied by `-W clippy::all`
> >>     = help: to override `-W clippy::all` add `#[allow(clippy::wrong_self_convention)]`
> >>
> >> https://lore.kernel.org/rust-for-linux/20250610132823.3457263-2-fujita.tomonori@gmail.com/
> >
> > Are we missing a derive(Copy) for this type? Clippy does not emit that
> > lint if the type is Copy:
> > https://github.com/rust-lang/rust-clippy/issues/273
>
> I think that both Delta and Instant structs implement Copy.
>
> #[repr(transparent)]
> #[derive(PartialEq, PartialOrd, Eq, Ord)]
> pub struct Instant<C: ClockSource> {
>     inner: bindings::ktime_t,
>     _c: PhantomData<C>,
> }
>
> impl<C: ClockSource> Clone for Instant<C> {
>     fn clone(&self) -> Self {
>         *self
>     }
> }
>
> impl<C: ClockSource> Copy for Instant<C> {}
>
> #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
> pub struct Delta {
>     nanos: i64,
> }
>
> The above warning is about the trait method.

Wait, it's a trait method!?

Alice

On Tue, 24 Jun 2025 14:54:24 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> >> So would the function be defined like this?
>> >>
>> >> fn as_nanos(self) -> i64;
>> >>
>> >> If that's the case, then we've come full circle back to the original
>> >> problem; Clippy warns against using as_* names for trait methods that
>> >> take self as follows:
>> >>
>> >> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
>> >>    --> /home/fujita/git/linux-rust/rust/kernel/time/hrtimer.rs:430:17
>> >>     |
>> >> 430 |     fn as_nanos(self) -> i64;
>> >>     |                 ^^^^
>> >>     |
>> >>     = help: consider choosing a less ambiguous name
>> >>     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
>> >>     = note: `-W clippy::wrong-self-convention` implied by `-W clippy::all`
>> >>     = help: to override `-W clippy::all` add `#[allow(clippy::wrong_self_convention)]`
>> >>
>> >> https://lore.kernel.org/rust-for-linux/20250610132823.3457263-2-fujita.tomonori@gmail.com/
>> >
>> > Are we missing a derive(Copy) for this type? Clippy does not emit that
>> > lint if the type is Copy:
>> > https://github.com/rust-lang/rust-clippy/issues/273
>>
>> I think that both Delta and Instant structs implement Copy.
>>
>> #[repr(transparent)]
>> #[derive(PartialEq, PartialOrd, Eq, Ord)]
>> pub struct Instant<C: ClockSource> {
>>     inner: bindings::ktime_t,
>>     _c: PhantomData<C>,
>> }
>>
>> impl<C: ClockSource> Clone for Instant<C> {
>>     fn clone(&self) -> Self {
>>         *self
>>     }
>> }
>>
>> impl<C: ClockSource> Copy for Instant<C> {}
>>
>> #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
>> pub struct Delta {
>>     nanos: i64,
>> }
>>
>> The above warning is about the trait method.
> 
> Wait, it's a trait method!?

Yes. Clippy warns the following implementation:

pub trait HrTimerExpires {
    fn as_nanos(self) -> i64;
}

Clippy doesn't warn when the methods on Delta and Instant are written
similarly. So Clippy is happy about the followings:

pub trait HrTimerExpires {
    fn as_nanos(&self) -> i64;
}

impl HrTimerExpires for Delta {
    fn as_nanos(&self) -> i64 {
        Delta::as_nanos(*self)
    }
}

On Tue, Jun 24, 2025 at 3:14 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Tue, 24 Jun 2025 14:54:24 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> >> >> So would the function be defined like this?
> >> >>
> >> >> fn as_nanos(self) -> i64;
> >> >>
> >> >> If that's the case, then we've come full circle back to the original
> >> >> problem; Clippy warns against using as_* names for trait methods that
> >> >> take self as follows:
> >> >>
> >> >> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
> >> >>    --> /home/fujita/git/linux-rust/rust/kernel/time/hrtimer.rs:430:17
> >> >>     |
> >> >> 430 |     fn as_nanos(self) -> i64;
> >> >>     |                 ^^^^
> >> >>     |
> >> >>     = help: consider choosing a less ambiguous name
> >> >>     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
> >> >>     = note: `-W clippy::wrong-self-convention` implied by `-W clippy::all`
> >> >>     = help: to override `-W clippy::all` add `#[allow(clippy::wrong_self_convention)]`
> >> >>
> >> >> https://lore.kernel.org/rust-for-linux/20250610132823.3457263-2-fujita.tomonori@gmail.com/
> >> >
> >> > Are we missing a derive(Copy) for this type? Clippy does not emit that
> >> > lint if the type is Copy:
> >> > https://github.com/rust-lang/rust-clippy/issues/273
> >>
> >> I think that both Delta and Instant structs implement Copy.
> >>
> >> #[repr(transparent)]
> >> #[derive(PartialEq, PartialOrd, Eq, Ord)]
> >> pub struct Instant<C: ClockSource> {
> >>     inner: bindings::ktime_t,
> >>     _c: PhantomData<C>,
> >> }
> >>
> >> impl<C: ClockSource> Clone for Instant<C> {
> >>     fn clone(&self) -> Self {
> >>         *self
> >>     }
> >> }
> >>
> >> impl<C: ClockSource> Copy for Instant<C> {}
> >>
> >> #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
> >> pub struct Delta {
> >>     nanos: i64,
> >> }
> >>
> >> The above warning is about the trait method.
> >
> > Wait, it's a trait method!?
>
> Yes. Clippy warns the following implementation:
>
> pub trait HrTimerExpires {
>     fn as_nanos(self) -> i64;
> }
>
> Clippy doesn't warn when the methods on Delta and Instant are written
> similarly. So Clippy is happy about the followings:
>
> pub trait HrTimerExpires {
>     fn as_nanos(&self) -> i64;
> }
>
> impl HrTimerExpires for Delta {
>     fn as_nanos(&self) -> i64 {
>         Delta::as_nanos(*self)
>     }
> }

If it's a trait method, then it should take &self because you don't
know whether it's Copy.

Alice

On Tue, 24 Jun 2025 15:45:45 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

> On Tue, Jun 24, 2025 at 3:14 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> On Tue, 24 Jun 2025 14:54:24 +0100
>> Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> >> >> So would the function be defined like this?
>> >> >>
>> >> >> fn as_nanos(self) -> i64;
>> >> >>
>> >> >> If that's the case, then we've come full circle back to the original
>> >> >> problem; Clippy warns against using as_* names for trait methods that
>> >> >> take self as follows:
>> >> >>
>> >> >> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
>> >> >>    --> /home/fujita/git/linux-rust/rust/kernel/time/hrtimer.rs:430:17
>> >> >>     |
>> >> >> 430 |     fn as_nanos(self) -> i64;
>> >> >>     |                 ^^^^
>> >> >>     |
>> >> >>     = help: consider choosing a less ambiguous name
>> >> >>     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
>> >> >>     = note: `-W clippy::wrong-self-convention` implied by `-W clippy::all`
>> >> >>     = help: to override `-W clippy::all` add `#[allow(clippy::wrong_self_convention)]`
>> >> >>
>> >> >> https://lore.kernel.org/rust-for-linux/20250610132823.3457263-2-fujita.tomonori@gmail.com/
>> >> >
>> >> > Are we missing a derive(Copy) for this type? Clippy does not emit that
>> >> > lint if the type is Copy:
>> >> > https://github.com/rust-lang/rust-clippy/issues/273
>> >>
>> >> I think that both Delta and Instant structs implement Copy.
>> >>
>> >> #[repr(transparent)]
>> >> #[derive(PartialEq, PartialOrd, Eq, Ord)]
>> >> pub struct Instant<C: ClockSource> {
>> >>     inner: bindings::ktime_t,
>> >>     _c: PhantomData<C>,
>> >> }
>> >>
>> >> impl<C: ClockSource> Clone for Instant<C> {
>> >>     fn clone(&self) -> Self {
>> >>         *self
>> >>     }
>> >> }
>> >>
>> >> impl<C: ClockSource> Copy for Instant<C> {}
>> >>
>> >> #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
>> >> pub struct Delta {
>> >>     nanos: i64,
>> >> }
>> >>
>> >> The above warning is about the trait method.
>> >
>> > Wait, it's a trait method!?
>>
>> Yes. Clippy warns the following implementation:
>>
>> pub trait HrTimerExpires {
>>     fn as_nanos(self) -> i64;
>> }
>>
>> Clippy doesn't warn when the methods on Delta and Instant are written
>> similarly. So Clippy is happy about the followings:
>>
>> pub trait HrTimerExpires {
>>     fn as_nanos(&self) -> i64;
>> }
>>
>> impl HrTimerExpires for Delta {
>>     fn as_nanos(&self) -> i64 {
>>         Delta::as_nanos(*self)
>>     }
>> }
> 
> If it's a trait method, then it should take &self because you don't
> know whether it's Copy.

A trait method as_* should take &self. The same name of a method in a
structure which implements that trait can take self (if the structure
implements Copy and the cost is free) instead of &self, although it
doesn't match the same name of the trait method.

Also, the conversions table says that the ownership of as_* is
borrowed -> borrowed so neither of them matches the table.

Right?

On Thu, Jun 19, 2025 at 9:08 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> So would the function be defined like this?
>
> fn as_nanos(self) -> i64;
>
> If that's the case, then we've come full circle back to the original
> problem; Clippy warns against using as_* names for trait methods that
> take self as follows:
>
> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference

Yeah, the Clippy warning is indeed one more data point that the
guidelines are confusing to the point of having Clippy complain or,
more likely, the guidelines' intention is that we should just pick
`&self`.

If we decide to be OK with `self`s in the kernel for these cases, we
can simply disable the lint. Doing so means we lose the rest of the
checking for that lint, sadly.

And, yeah, we are indeed going in circles.

What I would normally suggest for cases like this is answering: what
would be the best for the kernel's particular case, regardless of
existing guidelines/lints? Then, if we think it is better to be
different, and there is enough justification to do so, then try to
mitigate the lose of the lints, talk to upstream, write our own
variation of the guidelines, etc.

So I would like to hear if anybody feels strongly about either
direction, i.e. any other pros/cons that we haven't thought of.

Thanks!

Cheers,
Miguel

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Thu, Jun 19, 2025 at 9:08 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> So would the function be defined like this?
>>
>> fn as_nanos(self) -> i64;
>>
>> If that's the case, then we've come full circle back to the original
>> problem; Clippy warns against using as_* names for trait methods that
>> take self as follows:
>>
>> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
>
> Yeah, the Clippy warning is indeed one more data point that the
> guidelines are confusing to the point of having Clippy complain or,
> more likely, the guidelines' intention is that we should just pick
> `&self`.
>
> If we decide to be OK with `self`s in the kernel for these cases, we
> can simply disable the lint. Doing so means we lose the rest of the
> checking for that lint, sadly.
>
> And, yeah, we are indeed going in circles.
>
> What I would normally suggest for cases like this is answering: what
> would be the best for the kernel's particular case, regardless of
> existing guidelines/lints? Then, if we think it is better to be
> different, and there is enough justification to do so, then try to
> mitigate the lose of the lints, talk to upstream, write our own
> variation of the guidelines, etc.
>
> So I would like to hear if anybody feels strongly about either
> direction, i.e. any other pros/cons that we haven't thought of.

The table at [1] seems to suggest `to_*` or `into_*` being the right
prefix for this situation. It does not fully match `to_*`, as the
conversion is not expensive. It does not match `into_*` as the type is
`Copy`.

I am leaning towards `to_*`, but no strong feelings against `into_*`.

I would not go with `as_*`, I would expect that to borrow.


Best regards,
Andreas Hindborg

On Thu, Jun 19, 2025 at 11:28 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> The table at [1] seems to suggest `to_*` or `into_*` being the right
> prefix for this situation. It does not fully match `to_*`, as the
> conversion is not expensive. It does not match `into_*` as the type is
> `Copy`.
>
> I am leaning towards `to_*`, but no strong feelings against `into_*`.
>
> I would not go with `as_*`, I would expect that to borrow.

It is an integer division by compile-time constant, so likely just a
multiplication and some adjustment, so it depends on whether we
consider that "expensive".

However, even if we consider that "expensive", we will still have the
same question when we have a really cheap method.

The root issue is that the table just doesn't say what to do in some
of the "free" cases, and it is generally confusing.

Since I am asking for opinions: why do you consider `as_*` as
expecting to borrow? The standard does take `&self` the majority of
the time (but not always), and Clippy also expects a borrow, but you
also said in a previous iteration that you don't want to take a
pointer just to pass an integer, which makes sense: we wouldn't pass a
reference if we were using the integer.

Thanks!

(I am tempted to propose a new table...)

Cheers,
Miguel

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Thu, Jun 19, 2025 at 11:28 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> The table at [1] seems to suggest `to_*` or `into_*` being the right
>> prefix for this situation. It does not fully match `to_*`, as the
>> conversion is not expensive. It does not match `into_*` as the type is
>> `Copy`.
>>
>> I am leaning towards `to_*`, but no strong feelings against `into_*`.
>>
>> I would not go with `as_*`, I would expect that to borrow.
>
> It is an integer division by compile-time constant, so likely just a
> multiplication and some adjustment, so it depends on whether we
> consider that "expensive".
>
> However, even if we consider that "expensive", we will still have the
> same question when we have a really cheap method.
>
> The root issue is that the table just doesn't say what to do in some
> of the "free" cases, and it is generally confusing.
>
> Since I am asking for opinions: why do you consider `as_*` as
> expecting to borrow?

1) I **feel** that is usually the case. I did not check `std` if this
also the case in practice.
2) The table at [1] says `as_*` is borrowed -> borrowed.
3) To me, the wording "as" indicates a view into something.

> The standard does take `&self` the majority of
> the time (but not always), and Clippy also expects a borrow, but you
> also said in a previous iteration that you don't want to take a
> pointer just to pass an integer, which makes sense: we wouldn't pass a
> reference if we were using the integer.

Yes, I would prefer taking by value. I think Alice mentioned earlier in
this thread that the compiler will be smart about this and just pass the
value. But it still feels wrong to me.


Best regards,
Andreas Hindborg

On Thu, Jun 19, 2025 at 2:51 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Yes, I would prefer taking by value. I think Alice mentioned earlier in
> this thread that the compiler will be smart about this and just pass the
> value. But it still feels wrong to me.

If inlined/private, yes; but not always.

So for small ("free") functions like this, it should generally not
matter, since they should be inlined whether by manual marking or by
the compiler.

But, in general, it is not the same, and you can see cases where the
compiler will still pass a pointer, and thus dereferences and writes
to memory to take an address to pass it.

Which means that, outside small things like `as_*`, one should still
probably take by value. Which creates an inconsistency.

Cheers,
Miguel