Re: [PATCH] kernel.h: Add for_each_if()

[NeilBrown]

On Tue, Jul 10 2018, Daniel Vetter wrote:

> On Mon, Jul 09, 2018 at 04:30:01PM -0700, Andrew Morton wrote:
>> On Mon,  9 Jul 2018 18:25:09 +0200 Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
>> 
>> > To avoid compilers complainig about ambigious else blocks when putting
>> > an if condition into a for_each macro one needs to invert the
>> > condition and add a dummy else. We have a nice little convenience
>> > macro for that in drm headers, let's move it out. Subsequent patches
>> > will roll it out to other places.
>> > 
>> > The issue the compilers complain about are nested if with an else
>> > block and no {} to disambiguate which if the else belongs to. The C
>> > standard is clear, but in practice people forget:
>> > 
>> > if (foo)
>> > 	if (bar)
>> > 		/* something */
>> > 	else
>> > 		/* something else
>> 
>> um, yeah, don't do that.  Kernel coding style is very much to do
>> 
>> 	if (foo) {
>> 		if (bar)
>> 			/* something */
>> 		else
>> 			/* something else
>> 	}
>> 
>> And if not doing that generates a warning then, well, do that.
>> 
>> > The same can happen in a for_each macro when it also contains an if
>> > condition at the end, except the compiler message is now really
>> > confusing since there's only 1 if:
>> > 
>> > for_each_something()
>> > 	if (bar)
>> > 		/* something */
>> > 	else
>> > 		/* something else
>> > 
>> > The for_each_if() macro, by inverting the condition and adding an
>> > else, avoids the compiler warning.
>> 
>> Ditto.
>> 
>> > Motivated by a discussion with Andy and Yisheng, who want to add
>> > another for_each_macro which would benefit from for_each_if() instead
>> > of hand-rolling it.
>> 
>> Ditto.
>> 
>> > v2: Explain a bit better what this is good for, after the discussion
>> > with Peter Z.
>> 
>> Presumably the above was discussed in whatever-thread-that-was.
>
> So there's a bunch of open coded versions of this already in kernel
> headers (at least the ones I've found). Not counting the big pile of
> existing users in drm. They are all wrong and should be reverted to a
> plain if? That why there's a bunch more patches in this series.
>
> And yes I made it clear in the discussion that if you sprinkle enough {}
> there's no warning, should have probably captured this here.
>
> Aka a formal Nack-pls-keep-your-stuff-in-drm: would be appreciated so I
> can stop bothering with this.

I think is it problematic to have macros like

#define for_each_foo(...) for (......) if (....)

because
   for_each_foo(...)
      if (x) ....; else ......;

is handled badly.
So in that sense, your work seems like a good thing.

However it isn't clear to me that you need a new macro.
The above macro could simply be changed to

#define for_each_foo(...) for (......) if (!....);else

Clearly people don't always think to do this, but would adding a macro
help people to think?

If we were to have a macro, it isn't clear to me that for_each_if() is a
good name.
Every other macro I've seen that starts "for_each_" causes the body to
loop.  This one doesn't.  If someone doesn't know what for_each_if()
does and sees it in code, they are unlikely to jump to the right
conclusion.
I would suggest that "__if" would be a better choice.  I think most
people would guess that means "like 'if', but a bit different", which is
fairly accurate.

I think the only sure way to avoid bad macros being written is to teach
some static checker to warn about any macro with a dangling "if".
Possibly checkpatch.pl could do that (but I'm not volunteering).

I do agree that it would be good to do something, and if people find
for_each_fi() to actually reduce the number of poorly written macros,
then I don't object to it.

Thanks,
NeilBrown

Attachment: signature.asc
Description: PGP signature