On Wed, Feb 07, 2024 at 08:05:29AM -0500, Brian Foster wrote:
> On Tue, Feb 06, 2024 at 05:37:22PM -0500, Kent Overstreet wrote:
> > On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote:
> > > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote:
> > > > +static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > > +{
> > > > + struct super_block *sb = file_inode(file)->i_sb;
> > > > +
> > > > + if (!sb->s_uuid_len)
> > > > + return -ENOIOCTLCMD;
> > > > +
> > > > + struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > > > + memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
> > > > +
> > > > + return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> > > > +}
> > >
> > > Can we please keep the declarations separate from the code? I always
> > > find this sort of implicit scoping of variables both difficult to
> > > read (especially in larger functions) and a landmine waiting to be
> > > tripped over. This could easily just be:
> > >
> > > static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > {
> > > struct super_block *sb = file_inode(file)->i_sb;
> > > struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > >
> > > ....
> > >
> > > and then it's consistent with all the rest of the code...
> >
> > The way I'm doing it here is actually what I'm transitioning my own code
> > to - the big reason being that always declaring variables at the tops of
> > functions leads to separating declaration and initialization, and worse
> > it leads people to declaring a variable once and reusing it for multiple
> > things (I've seen that be a source of real bugs too many times).
> >
>
> I still think this is of questionable value. I know I've mentioned
> similar concerns to Dave's here on the bcachefs list, but still have not
> really seen any discussion other than a bit of back and forth on the
> handful of generally accepted (in the kernel) uses of this sort of thing
> for limiting scope in loops/branches and such.
>
> I was skimming through some more recent bcachefs patches the other day
> (the journal write pipelining stuff) where I came across one or two
> medium length functions where this had proliferated, and I found it kind
> of annoying TBH. It starts to almost look like there are casts all over
> the place and it's a bit more tedious to filter out logic from the
> additional/gratuitous syntax, IMO.
>
> That's still just my .02, but there was also previous mention of
> starting/having discussion on this sort of style change. Is that still
> the plan? If so, before or after proliferating it throughout the
> bcachefs code? ;) I am curious if there are other folks in kernel land
> who think this makes enough sense that they'd plan to adopt it. Hm?
That was the discussion :)
bcachefs is my codebase, so yes, I intend to do it there. I really think
this is an instance where you and Dave are used to the way C has
historically forced us to do things; our brains get wired to read code a
certain way and changes are jarring.
But take a step back; if we were used to writing code the way I'm doing
it, and you were arguing for putting declarations at the tops of
functions, what would the arguments be?
I would say you're just breaking up the flow of ideas for no reason; a
chain of related statements now includes a declaration that isn't with
the actual logic.
And bugs due to variable reuse, missed initialization - there's real
reasons not to do it that way.
