Re: guard coding style (was: Re: [PATCH v1 05/10] gpio: pca953x: Simplify code with cleanup helpers)

[Mitchell Levy <levymitchell0@xxxxxxxxx>]

Hey all,
A brief disclaimer, I'm a fairly new kernel contributor, but since I
was cc'd directly, I figured I might as well drop into the
conversation.

On Thu, Sep 14, 2023 at 12:47 AM Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Bartosz,
>
> On Wed, Sep 13, 2023 at 5:27 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> > On Wed, Sep 13, 2023 at 4:35 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > On Fri, 1 Sep 2023, Andy Shevchenko wrote:
> > > > Use macros defined in linux/cleanup.h to automate resource lifetime
> > > > control in gpio-pca953x.
> > > >
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > >
> > > Thanks for your patch, which is now commit 8e471b784a720f6f
> > > ("gpio: pca953x: Simplify code with cleanup helpers") in
> > > gpio/gpio/for-next.
> > >
> > > > --- a/drivers/gpio/gpio-pca953x.c
> > > > +++ b/drivers/gpio/gpio-pca953x.c
> > > > @@ -557,9 +554,8 @@ static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
> > > >       u32 reg_val;
> > > >       int ret;
> > > >
> > > > -     mutex_lock(&chip->i2c_lock);
> > > > -     ret = regmap_read(chip->regmap, inreg, &reg_val);
> > > > -     mutex_unlock(&chip->i2c_lock);
> > > > +     scoped_guard(mutex, &chip->i2c_lock)
> > > > +             ret = regmap_read(chip->regmap, inreg, &reg_val);
> > >
> > > I can't say I'm thrilled about the lack of curly braces.  I was also
> > > surprised to discover that checkpatch nor gcc W=1 complain about the
> > > indentation change.
> > > I know we don't use curly braces in single-statement for_each_*() loops,
> > > but at least these have the familiar "for"-prefix.  And having the scope
> > > is very important here, so using braces, this would stand out more.
> > >
> > > Hence can we please get curly braces, like
> > >
> > >      scoped_guard(mutex, &chip->i2c_lock) {
> > >             ret = regmap_read(chip->regmap, inreg, &reg_val);
> > >      }
> > >
> > > ?
> > >
> > > Thanks! ;-)
> >
> > I strongly disagree. The scope here is very clear - just like it is in
> > a for loop, in a while loop or in an if block:
> >
> > if (foo)
> >     bar()
> >
> > if (foo) {
> >     bar();
> >     baz();
> > }
> >
> > Only compound statements need curly braces in the kernel and it has
> > been like this forever. I don't really see a need to make it an
> > exception.

The more I think on this issue, the more I go back and forth. If we
only had guard(...), the only way to approximate scoped guard would be
to either just do what the macro does (i.e., a dummy for loop that
only runs once) or use an anonymous scope, e.g.,
{
    guard(...);
    my_one_statement();
}
Since this is how I've previously used std::lock_guard in C++, this
pattern feels very familiar to me, and the scoped_guard feels almost
like syntax sugar for this. As such, I feel like including the braces
is most natural because, as Geert mentioned, it emphasizes the scope
that "should" (in my brain, at least) be there.

Thanks,
Mitchell

> > That being said - I don't think the coding style for guard has ever
> > been addressed yet, so maybe bring it up with Peter Zijlstra?
>
> That's a good idea!
>
> I see Peter always used curly braces (but he didn't have any
> single-statement blocks, except for one with an "if", and we do tend
> to use curly braces in "for"-statements containing a single "if", too),
> but he does put a space after the "scoped_guard", as is also
> shown in the template in include/linux/cleanup.h:
>
>     scoped_guard (name, args...) { }:
>
> Then, "guard" does not get a space (but it is funny syntax
> anyway, with the double set of parentheses ;-).  The template in
> include/linux/cleanup.h doesn't match actual usage as it lacks the
> second set of parentheses:
>
>     guard(name):
>
> Peter: care to comment?
> Or do you have a different bikeshed to paint today? ;-)
>
> Thanks!
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds