[Intel-gfx] [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Dave Gordon]

On 25/11/15 09:23, Daniel Vetter wrote:
> On Tue, Nov 24, 2015 at 11:47:26PM +0000, Chris Wilson wrote:
>> On Tue, Nov 24, 2015 at 10:26:01PM +0000, Chris Wilson wrote:
>>> On Tue, Nov 24, 2015 at 07:36:25PM +0200, Jani Nikula wrote:
>>>>   /* Iterate over initialised rings */
>>>>   #define for_each_ring(ring__, dev_priv__, i__) \
>>>>   	for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
>>>> -		if (((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__)))
>>>> +		for_each_if ((((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__))))
>>>
>>> Idly wondering if we would be happy with
>>>
>>> for_each_ring(ring__, dev_priv__)
>>> 	for ((ring__) = &(dev_priv__)->ring[0];
>>> 	     (ring__) <= &(dev_priv__)->ring[I915_NUM_RINGS];
>>> 	     (ring__)++)
>>> 	     for_each_if(intel_ring_initialized(ring__))
>>>
>>> ?
>>>
>>> The downside is that we have used i__ in several places rather than
>>> ring->id.
>>
>> Fwiw, 13 files changed, 113 insertions(+), 140 deletions(-)
>>
>> Seems a reasonable shrinkage.
>
> Maybe for_each_engine even, and phase out for_each_ring completely?
> -Daniel
>

Wouldn't it be nicer (and safer) not to build macros that fold the loop 
structure into the macro (in contravention of kernel programming 
guidelines).

So how about NOT including the actual for() inside the macro, so that 
instead of writing

	for_each_engine(engine, dev_priv)
		do_stuff(engine)

we would write it as

	for (EACH_ENGINE(engine, dev_priv))
		initialise(engine)

	for (EACH_ACTIVE_ENGINE(engine, dev_priv)) {
		service(engine)
		restart(engine)
	}

etc. The for-loop is visible and the scope doesn't give you any surprises.

[The EACH_ENGINE() macros expands to a semicolon-separated triplet of 
expressions; still a violation of the "don't use macros to redefine C 
syntax" guideline, but much less egregious than macros that contain 
embedded 'for's and 'if's.

.Dave.