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

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]

On Wed, Jul 11 2018, Andrew Morton wrote:

> On Wed, 11 Jul 2018 13:51:08 +0200 Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> But I still have the situation that a bunch of maintainers acked this
>> and Andrew Morton defacto nacked it, which I guess means I'll keep the
>> macro in drm? The common way to go about this seems to be to just push
>> the patch series with the ack in some pull request to Linus and ignore
>> the people who raised questions, but not really my thing.
>
> Heh.
>
> But, am I wrong?  Code which uses regular kernel style doesn't have
> these issues.  We shouldn't be enabling irregular style - we should be
> making such sites more regular.  The fact that the compiler generates a
> nice warning in some cases simply helps us with that.

I think you are wrong .... or at least, not completely correct.

I think it is perfectly acceptable in Linux to have code like:

  for (....)
  	if (x)
        	something();
        else
        	something_else();

Would you agree?  If not, then I'm the one who is wrong.  Otherwise....

The problem is that for certain poorly written for_each_foo() macros,
such as blkg_for_each_descendant_pre() (and several others identified in
this patch series), writing

   blkg_for_each_descendant_pre(...)
     	if (x)
        	something();
        else
        	something_else();

will trigger a compiler warning.  This is inconsistent with the
behaviour of a simple "for".
So I do think that the macros should be fixed, and I don't think that
sprinkling extra braces is an appropriate response.

I'm not personally convinced that writing
   if_no_else(cond)
is easier than just writing
   if (!(cond)); else

in these macros, but I do think that the macros should be fixed and
maybe this is the path-of-least-resistance to getting it done.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]