On Tue, Nov 12, 2024 at 11:15:54AM +0100, Uwe Kleine-König wrote:
> Hello Andy, hello Aren,
>
> On Mon, Nov 11, 2024 at 11:44:51AM +0200, Andy Shevchenko wrote:
> > On Sun, Nov 10, 2024 at 04:34:30PM -0500, Aren wrote:
> > > On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote:
> > > > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti:
> >
> > You can do it differently
> >
> > #define STK3310_REGFIELD(name) \
> > do { \
> > data->reg_##name = \
> > devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_##name); \
> > if (IS_ERR(data->reg_##name)) \
> > return dev_err_probe(dev, PTR_ERR(data->reg_##name), \
> > "reg field alloc failed.\n"); \
> > } while (0)
> >
> > > #define STK3310_REGFIELD(name) ({ \
> > > data->reg_##name = devm_regmap_field_alloc(dev, regmap, \
> > > stk3310_reg_field_##name); \
> > > if (IS_ERR(data->reg_##name)) \
> > > return dev_err_probe(dev, PTR_ERR(data->reg_##name), \
> > > "reg field alloc failed\n"); \
> > > })
> >
> > I am against unneeded use of GNU extensions.
> >
> > > > > replacing "do { } while (0)" with "({ })" and deindenting could make
> > > > > enough room to clean this up the formatting of this macro though.
> > > >
> > > > do {} while (0) is C standard, ({}) is not.
> > >
> > > ({ }) is used throughout the kernel, and is documented as such[1]. I
> > > don't see a reason to avoid it, if it helps readability.
> >
> > I don't see how it makes things better here, and not everybody is familiar with
> > the concept even if it's used in the kernel here and there. Also if a tool is
> > being used in one case it doesn't mean it's suitable for another.
>
> Just to throw in my subjective view here: I don't expect anyone with
> some base level knowledge of C will have doubts about the semantics of
> ({ ... }) and compared to that I find do { ... } while (0) less optimal,
> because it's more verbose and when spotting the "do {" part, the
> semantic only gets clear when you also see the "while (0)". Having said
> that I also dislike the "do" starting on column 0, IMHO the RHS of the
> #define should be intended.
Thank you, this sums up my opinion on this better than I could have (and
some bits I hadn't considered).
> So if you ask me, this is not an unneeded use of an extension. The
> extension is used to improve readabilty and I blame the C standard to
> not support this syntax.
>
> While I'm in critics mode: I consider hiding a return in a macro bad
> style.
Yeah... probably worse than any of the formatting options here. I guess
the proper way would be to use devm_regmap_field_bulk_alloc, but that's
well outside the scope of this series. Perhaps it would make sense to
move the macro definition to just before the function it's used in so
it's at least a little easier to spot?
- Aren
