On Tue, Mar 26, 2024 at 8:41 AM Philipp Stanner <pstanner@xxxxxxxxxx> 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
