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