On Thu, 2024-03-21 at 18:20 -0700, Boqun Feng wrote:
> On Thu, Mar 21, 2024 at 05:22:11PM -0700, John Hubbard wrote:
> > On 3/19/24 2:32 AM, Philipp Stanner wrote:
> > ...
> > > >
> > > > > + if ret < 0 {
> > > > > + Err(Error::from_errno(ret))
> > > > > + } else {
> > > > > + guard.dismiss();
> > > > > + Ok(id as usize)
> > > > > + }
> > > >
> > > > You could make this easier to read using to_result.
> > > >
> > > > to_result(ret)?;
> > > > guard.dismiss();
> > > > Ok(id as usize)
> > >
> > > My 2 cents, I'd go for classic kernel style:
> > >
> > >
> > > if ret < 0 {
> > > return Err(...);
> > > }
> > >
> > > guard.dismiss();
> > > Ok(id as usize)
> > >
> >
> > Yes.
> >
> > As a "standard" C-based kernel person who is trying to move into
> > Rust
> > for Linux, I hereby invoke my Rust-newbie powers to confirm that
> > the
> > "clasic kernel style" above goes into my head much faster and
> > easier,
> > yes. :)
> >
>
> Hope I'm still belong to a "standard C-based kernel person" ;-) My
> problem on "if ret < 0 { ... }" is: what's the type of "ret", and is
> it
> "negative means failure" or "non-zero means failure"? Now for this
> particular code, the assignment of "ret" and the use of "ret" is
> pretty
> close, so it doesn't matter. But in the code where "ret" is used for
> multiple function calls and there is code in-between, then I'd prefer
> we
> use `to_result` (i.e. `Result` type and question mark operator).
Yeah, the issue we're running into here is that it's a quite different
way of programming since you create bindings on the fly with `let`.
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.
Regarding our example, my hope would be that `if ret < 0 ` should
always be fine, because, what else could it be? A float?
And such patterns should only appear where you interact with C, since
pure Rust should definitely always have all result status packed in
Result or Option.
>
> > I hope I'm not violating any "this is how Rust idioms must be"
> > conventions. But if not, then all other things being equal, it is
> > of
> > course a nice touch to make the code more readable to the rest of
> > the
> > kernel folks.
> >
>
> One more extra point from myself only: if one is using Rust for
> drivers
> or subsystem they are going to take care of it in the future, it's
> totally fine for them to pick coding styles that they feel
> comfortable,
> I don't want to make people who do the real work feel frustrated
> because
> "this is how Rust idioms must be", also I don't believe tools should
> restrict people.
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?
In any case I don't even think they shouldn't be global, but that they
should behave more like checkpatch.
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.
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
> But in the "kernel" crate (i.e. for core kernel part),
> I want to make it "Rusty" since it's the point of the experiement
> ("you
> asked for it ;-)).
>
> Hope we can find a balanced point when we learn more and more ;-)
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
Many greetings,
P.
>
> Regards,
> Boqun
>
> >
> > thanks,
> > --
> > John Hubbard
> > NVIDIA
> >
>
