| Message ID | 20180709162509.29343-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2018-07-09 at 18:25 +0200, Daniel Vetter wrote: > v2: Explain a bit better what this is good for, after the discussion > with Peter Z. Since I have not been Cc'ed to your discussion there is another weirdness which this macro prohibits, i.e. for_each_blah() { } else { ...blahblah... }
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.
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
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@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. 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
On Tue, Jul 10, 2018 at 12:32 PM, NeilBrown <neilb@suse.com> wrote: > 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@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. > > 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. There's also the proposal of if_noelse() which I think fares a bit better than the __if(). 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. -Daniel
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.
On Thu, Jul 12, 2018 at 1:05 AM, Andrew Morton <akpm@linux-foundation.org> 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. Yes liberal sprinkling of {} per coding style fixes it too. But given that the if (!cond) ; else pattern is fairly common even outside of drm it seems like not just graphics people think that little bit of added robustness in iterator macros is worth it. Only reason I bothered with this is because I've seen another open-coded version of this pattern fly by. Anyway I'm going on vacations for a few weeks now. Andy/Yisheng, I guess if you want this it's up to you to haggle for a consensus around this. If that doesn't happen I'm happy to keep it in drm headers. -Daniel
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
On 07/13/2018 04:37 PM, NeilBrown wrote: > 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.... coding-style.rst says: Also, use braces when a loop contains more than a single simple statement: > 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 agreed. > 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. I'm not opposed to fixing some macros, but some of these macros are just ease-of-less-typing shortcuts. They don't improve readability at all; they harm it. (of course, that is just one opinion :)
On Fri, 2018-07-13 at 16:42 -0700, Randy Dunlap wrote: > On 07/13/2018 04:37 PM, NeilBrown wrote: > > coding-style.rst says: > Also, use braces when a loop contains more than a single simple > statement: Independently on a) would we use some macro for condition, or b) fix macros against this kind of nested conditions, there is another weirdness we would like to avoid, i.e. for_each_foo() { ... } else { ... } It is written according to coding style, but too much weird. So, summarize this discussion I think we would - keep for_each_if() in DRM subsystem alone - fix macros which are using positive condition 'if (cond)' by replacing with 'if (!cond) {} else' form for sake of robustness. Do you agree on that?
On 07/16/2018 01:11 AM, Andy Shevchenko wrote: > On Fri, 2018-07-13 at 16:42 -0700, Randy Dunlap wrote: >> On 07/13/2018 04:37 PM, NeilBrown wrote: > >> >> coding-style.rst says: >> Also, use braces when a loop contains more than a single simple >> statement: > > Independently on a) would we use some macro for condition, or b) fix > macros against this kind of nested conditions, there is another > weirdness we would like to avoid, i.e. > > for_each_foo() { > ... > } else { > ... > } > > It is written according to coding style, but too much weird. Yeah, that's odd. Looks like else matches the for loop. (!) > So, summarize this discussion I think we would > - keep for_each_if() in DRM subsystem alone > - fix macros which are using positive condition 'if (cond)' by replacing > with 'if (!cond) {} else' form for sake of robustness. > > Do you agree on that? Sure, both of those sound good to me. thanks,
On Mon, Jul 16 2018, Andy Shevchenko wrote: > On Fri, 2018-07-13 at 16:42 -0700, Randy Dunlap wrote: >> On 07/13/2018 04:37 PM, NeilBrown wrote: > >> >> coding-style.rst says: >> Also, use braces when a loop contains more than a single simple >> statement: > > Independently on a) would we use some macro for condition, or b) fix > macros against this kind of nested conditions, there is another > weirdness we would like to avoid, i.e. > > for_each_foo() { > ... > } else { > ... > } > > It is written according to coding style, but too much weird. Agreed. Too weird. > > So, summarize this discussion I think we would > - keep for_each_if() in DRM subsystem alone > - fix macros which are using positive condition 'if (cond)' by replacing > with 'if (!cond) {} else' form for sake of robustness. > > Do you agree on that? I agree. Thanks, NeilBrown
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index f7a19c2a7a80..05350424a4d3 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -110,7 +110,4 @@ static inline bool drm_can_sleep(void) return true; } -/* helper for handling conditionals in various for_each macros */ -#define for_each_if(condition) if (!(condition)) {} else - #endif diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 941dc0a5a877..4cb95ab9a5bc 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -71,6 +71,9 @@ */ #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr)) +/* helper for handling conditionals in various for_each macros */ +#define for_each_if(condition) if (!(condition)) {} else + #define u64_to_user_ptr(x) ( \ { \ typecheck(u64, x); \
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 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. 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. Cc: Gustavo Padovan <gustavo@padovan.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Sean Paul <seanpaul@chromium.org> Cc: David Airlie <airlied@linux.ie> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Kees Cook <keescook@chromium.org> Cc: Ingo Molnar <mingo@kernel.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: NeilBrown <neilb@suse.com> Cc: Wei Wang <wvw@google.com> Cc: Stefan Agner <stefan@agner.ch> Cc: Andrei Vagin <avagin@openvz.org> Cc: Randy Dunlap <rdunlap@infradead.org> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Yisheng Xie <ysxie@foxmail.com> Cc: Peter Zijlstra <peterz@infradead.org> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> -- v2: Explain a bit better what this is good for, after the discussion with Peter Z. --- include/drm/drmP.h | 3 --- include/linux/kernel.h | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-)