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

From: Daniel Vetter <daniel@ffwll.ch>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	LKML <linux-kernel@vger.kernel.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Gustavo Padovan <gustavo@padovan.org>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Sean Paul <seanpaul@chromium.org>,
	David Airlie <airlied@linux.ie>,
	Kees Cook <keescook@chromium.org>, Ingo Molnar <mingo@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	NeilBrown <neilb@suse.com>, Wei Wang <wvw@google.com>,
	Stefan Agner <stefan@agner.ch>, Andrei Vagin <avagin@openvz.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Yisheng Xie <ysxie@foxmail.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH] kernel.h: Add for_each_if()
Date: Tue, 10 Jul 2018 09:53:28 +0200	[thread overview]
Message-ID: <20180710075328.GG3008@phenom.ffwll.local> (raw)
In-Reply-To: <20180709163001.8fb8148223a57bc46a13fbda@linux-foundation.org>

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@ffwll.ch> 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.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch