Message-ID: <CANiq72kYzx7JTVqrJuP0Wo9=1qtaN7s7fqkD5DDcjA59SgMizQ@mail.gmail.com>
Date: Fri, 21 Mar 2025 19:49:57 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Yury Norov <yury.norov@...il.com>, David Gow <davidgow@...gle.com>
Cc: Burak Emir <bqe@...gle.com>, Rasmus Villemoes <linux@...musvillemoes.dk>, 
	Viresh Kumar <viresh.kumar@...aro.org>, 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>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/4] rust: add bitmap API.

Hi Yury,

A couple comments in case it helps regarding the docs/comments discussion...

On Fri, Mar 21, 2025 at 5:04 PM Yury Norov <yury.norov@...il.com> wrote:
>
> In C we separate declarations from function body with an empty line.
> Can you do that in rust? Can you point to a rust coding style? Do you
> guys really use 2-whitespace tabs?

Please see https://docs.kernel.org/rust/coding-guidelines.html.

You are right that the example block you quoted should have the
formatting fixed.

I am not sure what you mean by separating declarations from body here.
I guess you mean variable declarations in C vs. the rest of the body
-- in Rust it is not done, i.e. declaring new bindings in the middle
as they are needed is expected (and sometimes needed).

> I think I already asked to make the test a separate unit. It's amazing
> that rust understands scattered commented blocks of code and can turn
> them into unit tests. Unfortunately, I'm not.
>
> Please create a separate unit and test everything there, just like we
> do with normal C code.

APIs should have examples, ideally good ones etc. The above looks like
an standard example, but maybe I am missing something.

The examples are meant to be documentation first and foremost, that
happens to double as a test (so that it does not get out of sync
etc.). It is how we document everything else in Rust, and in fact we
are becoming stricter in requesting more examples when people add more
APIs (otherwise they never get added :)

If actual tests are needed (i.e. on top of what examples provide),
then we just finally landed in -next `#[test]`-like support, i.e. unit
tests that can be written separately. We try to have tests as close as
possible to the code they exercise, too, but in some cases it may be
best to separate them (e.g. if there are way too many).

> For find_bit functions we have a lib/find_bit_benchmark.c Can you add
> a similar rust test, so we'll make sure you're not introducing
> performance regressions with your wrappers?
>
> Please don't use KUNITs. It's not ready for benchmarks, and tests built
> against it don't run on major distros.

Cc'ing David here in case he has more context around this...

I agree having a good "integrated benchmark" system would be nice, and
being able to mark particular "tests" as benchmarks etc.

Regarding distributions, it sounds an orthogonal issue to me, but I
may be lacking context...

> Are you sure a public method description should bear implementation
> details? I'm not. If implementation changes in future, the public API
> should stay stable (yes, including comments).

To clarify, it is describing the invariants of a type (i.e. it is not
a method description).

The invariants in some cases refer to private details (e.g. typically
a field), and whether to allow that or not is something we discussed
several times in the past.

We went with allowing the `# Invariants` section to refer to the
fields of a type if needed, as a practical decision. However, if
something is a "private invariant" that others must not rely on, then
we should still point it out explicitly etc.

I hope that clarifies.

Cheers,
Miguel

Powered by blists - more mailing lists