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 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