Re: [PATCH v5 3/4] rust: add bitmap API.

[Burak Emir]

On Fri, Mar 21, 2025 at 7:50 PM Miguel Ojeda
<miguel.ojeda.sandonis@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx> 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).
>

I tried this in today's mainline and got errors "undefined reference
to `bitmap_free', `bitmap_zalloc`..."
Is the version that landed in -next fixing those? Are there known limitations?

> > 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.

Thanks, it does & I will leave these as is then.