On Tue, 2024-03-26 at 13:23 +0100, Miguel Ojeda wrote: > 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. You mean if `ret` was a pointer that, consequently, would have to be checked for equality rather then for less-than 0? > > > 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? The difference is that checkpatch is an additional tool that is not tied to the compiler or build chain. > > 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). Yes, and I think that shouldn't be done. The C coding guideline is more of a recommendation. Nothing in C complains about you doing a `return;` over a `goto out;`. The current setting, however, would fire warnings (or isn't it even classified as a build error?) if you add `return` in Rust at the end of a code path. I believe that memory-safety-related mechanisms and rules should be "enforced as much as reasonably possible", because that's the entire point of Rust in the kernel. Everything else should in my opinion be as free as in C. > > > 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. Agreed, that should be global. In fact, almost everything should be global. The only thing we don't agree on is what should be a per-default strictly enforced rule, and what should be a recommendation. P. > > Cheers, > Miguel >
