Re: [PATCH v8 2/2] rust: xarray: Add an abstraction for XArray

[Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>]

On Tue, Mar 26, 2024 at 8:41 AM Philipp Stanner <pstanner@redhat.com> wrote:
>
> I actually think it would be nice to have variable declarations at the
> top of the functions, but that would obviously very frequently collide
> with Rust's concept of data being immutable by default.

It is true that deferring bindings sometimes allows for more
immutability (this also applies to kernel C to some degree with
variables at the top, by the way). However, it is more about other
things like when new objects can be constructed (when can invariants
be set), when destructors run and in what order, even the type itself
(rebinding).

But it is not just this (where to place variables/bindings) -- there
are other kernel C guidelines that do not make sense for Rust, and
trying to apply them is a bad idea.

Where possible and where it makes sense, we should try to keep it
consistent, of course. For instance, naming concerns have been
discussed many times (avoiding different names for existing things) --
that can be indeed be confusing and introduce mental overhead, and
thus we try to avoid it, even if sometimes it may have been locally
the best solution.

> Regarding our example, my hope would be that `if ret < 0 ` should
> always be fine, because, what else could it be? A float?

I think one of the things Boqun was referring to were the semantics of
the value, which is why one advantage of `to_result` is that, when
dealing with a C function that does not follow the usual pattern, it
stands out more.

> I'm very happy to hear that we're on sync here :)
>
> Though there will be a lot to be discussed and done. As I see it, so
> far the clippy rules for example are global, aren't they?

Yeah, and that is intentional.

Of course, as with anything else, there can be exceptions where they
make sense. But the point is to avoid diverging in how we write the
code in different parts of the kernel unless there is a good reason.

This is essentially the same as the formatting discussion. The flags
for `rustfmt`, Clippy or the compiler diagnostics can and will change
to try to come up with the optimal set as we learn more (and as new
flags appear or if they change behavior in a way that we didn't expect
-- hopefully not -- etc.), but they should be consistent for the
entire kernel, except where there is a good reason not to.

> I think the ideal end result would be one where we have rules and
> enforcement similar to how it's done in C:
> C has checkpatch, which fires errors for some things, and warnings for
> others. And, if it makes sense, you or the maintainer can ignore the
> warning.

I may not be understanding this part, but that is already the case, no?

In any case, I am not sure we should look at `checkpatch.pl` as a
reference here, because it is a tool that we apply against patches,
i.e. something done at a given point in time once against "ephemeral"
entities.

In other words, it is always possible to ignore any and all of its
warnings/errors, at the discretion of the maintainer, and thus it is
not as big of a deal to have false positives for the checks (in fact,
it is good that it does, because it allows `checkpatch.pl` more leeway
to catch more issues).

We could have different sets like with `W=`, though, including perhaps
one for "development" where it is even more relaxed (e.g. no missing
documentation warning).

> When the build chain checks global rules we can't really ignore any
> rule without some kind of annotation in the code, because otherwise
> we'd permanently spam the build log

That is the intention, i.e. that they are enforced as much as
reasonably possible, but one can still ignore the rule if really
needed (and even better, one can do it as locally as possible, e.g.
right at the item that requires it instead of file-wide).

> Yeah, I get where that comes from. Many new languages attempt to end
> endless style discussions etc. by axiomatically defining a canonical
> style.
>
> But we're living in this special world called kernel with its own laws
> of physics so to speak. To some degree we already have to use a
> "dialect / flavor" of Rust (CStr, try_push()?)
> Will be interesting to see how it all plays out for us once the users
> and use cases increase in number more and more

The "dialect" you are speaking about is global for the kernel. That is
completely fine, and as you say, needed in some aspects.

But what we do not want is to end up with different "dialects" within
the kernel, unless there is a very good reason for exceptional cases.

Cheers,
Miguel