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
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
