| Message ID | 20180709083650.23549-11-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 09, 2018 at 10:36:49AM +0200, Daniel Vetter wrote: > Avoids complaints from gcc about ambiguous else clauses. Is that a new thing? I'm fairly sure I've never seen it do that, > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Peter Zijlstra <peterz@infradead.org> > --- > include/linux/topology.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/topology.h b/include/linux/topology.h > index cb0775e1ee4b..4fba5a5b148d 100644 > --- a/include/linux/topology.h > +++ b/include/linux/topology.h > @@ -40,7 +40,7 @@ > > #define for_each_node_with_cpus(node) \ > for_each_online_node(node) \ > - if (nr_cpus_node(node)) > + for_each_if (nr_cpus_node(node)) Not having gotten any of the other patches, I'm not really sure what this does and such, but improve readability it does not :/
On Mon, Jul 9, 2018 at 12:36 PM, Peter Zijlstra <peterz@infradead.org> wrote: > On Mon, Jul 09, 2018 at 10:36:49AM +0200, Daniel Vetter wrote: >> Avoids complaints from gcc about ambiguous else clauses. > > Is that a new thing? I'm fairly sure I've never seen it do that, > >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: Peter Zijlstra <peterz@infradead.org> >> --- >> include/linux/topology.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/linux/topology.h b/include/linux/topology.h >> index cb0775e1ee4b..4fba5a5b148d 100644 >> --- a/include/linux/topology.h >> +++ b/include/linux/topology.h >> @@ -40,7 +40,7 @@ >> >> #define for_each_node_with_cpus(node) \ >> for_each_online_node(node) \ >> - if (nr_cpus_node(node)) >> + for_each_if (nr_cpus_node(node)) > > Not having gotten any of the other patches, I'm not really sure what > this does and such, but improve readability it does not :/ Patch 1 in this series, which I dumped onto lkml as a whole: https://lkml.org/lkml/2018/7/9/179 Imo it does improve readability for the if (!cond) {} else pattern. And (assuming my grep fu isn't too badly wrong) most places in the kernel do use this pattern in for_each macros, so I guess its a real thing. We've definitely hit it plenty in drm iterators (but we seem to like if() checks in iterator macros maybe a bit too much). I'm happy to drop this patch tough if you deem it offensive. -Daniel
On Mon, Jul 09, 2018 at 05:00:07PM +0200, Daniel Vetter wrote: > On Mon, Jul 9, 2018 at 12:36 PM, Peter Zijlstra <peterz@infradead.org> wrote: > > On Mon, Jul 09, 2018 at 10:36:49AM +0200, Daniel Vetter wrote: > >> #define for_each_node_with_cpus(node) \ > >> for_each_online_node(node) \ > >> - if (nr_cpus_node(node)) > >> + for_each_if (nr_cpus_node(node)) > > > > Not having gotten any of the other patches, I'm not really sure what > > this does and such, but improve readability it does not :/ > > Patch 1 in this series, which I dumped onto lkml as a whole: > > https://lkml.org/lkml/2018/7/9/179 Right, so while I don't object to being Cc'ed to the whole series, I do mind not being Cc'ed to at least the generic bits required to understand the patch I do have to look at. > Imo it does improve readability for the if (!cond) {} else pattern. > And (assuming my grep fu isn't too badly wrong) most places in the > kernel do use this pattern in for_each macros, so I guess its a real > thing. We've definitely hit it plenty in drm iterators (but we seem to > like if() checks in iterator macros maybe a bit too much). > > I'm happy to drop this patch tough if you deem it offensive. I'd just like to understand it better; what compiler complains about this and is the warning otherwise useful? These things don't seem mentioned in that initial patch either. IOW I suppose I'm asking for the justification of this churn. If it's really needed and useful so be it, but so far I'm not seeing any. At a while guess I'd say this is something new in gcc-8 (and while I have that installed on some machines, it doesn't seem to be the default, and so I've not actually seen its output). But is the warning actually useful, should we not just kill the warning like we tend to do some really silly ones.
On Mon, Jul 09, 2018 at 05:12:58PM +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 05:00:07PM +0200, Daniel Vetter wrote: > > On Mon, Jul 9, 2018 at 12:36 PM, Peter Zijlstra <peterz@infradead.org> wrote: > > > On Mon, Jul 09, 2018 at 10:36:49AM +0200, Daniel Vetter wrote: > > > >> #define for_each_node_with_cpus(node) \ > > >> for_each_online_node(node) \ > > >> - if (nr_cpus_node(node)) > > >> + for_each_if (nr_cpus_node(node)) > > > > > > Not having gotten any of the other patches, I'm not really sure what > > > this does and such, but improve readability it does not :/ > > > > Patch 1 in this series, which I dumped onto lkml as a whole: > > > > https://lkml.org/lkml/2018/7/9/179 > > Right, so while I don't object to being Cc'ed to the whole series, I do > mind not being Cc'ed to at least the generic bits required to understand > the patch I do have to look at. > > > Imo it does improve readability for the if (!cond) {} else pattern. > > And (assuming my grep fu isn't too badly wrong) most places in the > > kernel do use this pattern in for_each macros, so I guess its a real > > thing. We've definitely hit it plenty in drm iterators (but we seem to > > like if() checks in iterator macros maybe a bit too much). > > > > I'm happy to drop this patch tough if you deem it offensive. > > I'd just like to understand it better; what compiler complains about > this and is the warning otherwise useful? These things don't seem > mentioned in that initial patch either. > > IOW I suppose I'm asking for the justification of this churn. If it's > really needed and useful so be it, but so far I'm not seeing any. > > At a while guess I'd say this is something new in gcc-8 (and while I > have that installed on some machines, it doesn't seem to be the default, > and so I've not actually seen its output). But is the warning actually > useful, should we not just kill the warning like we tend to do some > really silly ones. for_each_something(foo) if (foo->bla) call_bla(foo); else call_default(foo); Totally contrived, but this complains. Liberally sprinkling {} also shuts up the compiler, but it's a bit confusing given that a plain for {;;} is totally fine. And it's confusing since at first glance the compiler complaining about nested if and ambigous else doesn't make sense since clearly there's only 1 if there. Wrt this being useful or not: We've had it for a while in drm, and Andy and Yishen where rolling yet another open coded version of this on a patch that flew past me on dri-devel. So I pointed them at the for_each_if() we have and typed this series to move it to kernel.h. -Daniel
On Mon, Jul 09, 2018 at 05:52:04PM +0200, Daniel Vetter wrote: > for_each_something(foo) > if (foo->bla) > call_bla(foo); > else > call_default(foo); > > Totally contrived, but this complains. Liberally sprinkling {} also shuts > up the compiler, but it's a bit confusing given that a plain for {;;} is > totally fine. And it's confusing since at first glance the compiler > complaining about nested if and ambigous else doesn't make sense since > clearly there's only 1 if there. Ah, so the pattern the compiler tries to warn about is: if (foo) if (bar) /* stmts1 */ else /* stmts2 * Because it might not be immediately obvious with which if the else goes. Which is fair enough I suppose. OK, ACK.
On Mon, Jul 9, 2018 at 6:03 PM, Peter Zijlstra <peterz@infradead.org> wrote: > On Mon, Jul 09, 2018 at 05:52:04PM +0200, Daniel Vetter wrote: >> for_each_something(foo) >> if (foo->bla) >> call_bla(foo); >> else >> call_default(foo); >> >> Totally contrived, but this complains. Liberally sprinkling {} also shuts >> up the compiler, but it's a bit confusing given that a plain for {;;} is >> totally fine. And it's confusing since at first glance the compiler >> complaining about nested if and ambigous else doesn't make sense since >> clearly there's only 1 if there. > > Ah, so the pattern the compiler tries to warn about is: > > if (foo) > if (bar) > /* stmts1 */ > else > /* stmts2 * > > Because it might not be immediately obvious with which if the else goes. > Which is fair enough I suppose. Yup. I'll augment the commit message of patch 1 to include this as example, and why it's confusing in context of a for_each_foo macro containing an if(). > OK, ACK. Thanks, Daniel
On Mon, Jul 09, 2018 at 06:03:42PM +0200, Peter Zijlstra wrote: > On Mon, Jul 09, 2018 at 05:52:04PM +0200, Daniel Vetter wrote: > > for_each_something(foo) > > if (foo->bla) > > call_bla(foo); > > else > > call_default(foo); > > > > Totally contrived, but this complains. Liberally sprinkling {} also shuts > > up the compiler, but it's a bit confusing given that a plain for {;;} is > > totally fine. And it's confusing since at first glance the compiler > > complaining about nested if and ambigous else doesn't make sense since > > clearly there's only 1 if there. > > Ah, so the pattern the compiler tries to warn about is: > > if (foo) > if (bar) > /* stmts1 */ > else > /* stmts2 * > > Because it might not be immediately obvious with which if the else goes. > Which is fair enough I suppose. > > OK, ACK. Just to bikeshed, there could be macros other than for_each_*() macros that will want to use this internally, so perhaps it would be worth the generic version being named something like if_noelse(). We could always add that as/when required, though. Mark.
On Mon, Jul 09, 2018 at 05:52:04PM +0200, Daniel Vetter wrote: > for_each_something(foo) > if (foo->bla) > call_bla(foo); > else > call_default(foo); Note that the kernel coding style 'discourages' this style and would like you to write: for_each_something(foo) { if (foo->bla) call_bla(foo); else call_default(foo); } Which avoids the entire problem. See CodingStyle-3: Also, use braces when a loop contains more than a single simple statement: .. code-block:: c while (condition) { if (test) do_something(); }
On Mon, Jul 9, 2018 at 6:12 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Mon, Jul 09, 2018 at 06:03:42PM +0200, Peter Zijlstra wrote: >> On Mon, Jul 09, 2018 at 05:52:04PM +0200, Daniel Vetter wrote: >> > for_each_something(foo) >> > if (foo->bla) >> > call_bla(foo); >> > else >> > call_default(foo); >> > >> > Totally contrived, but this complains. Liberally sprinkling {} also shuts >> > up the compiler, but it's a bit confusing given that a plain for {;;} is >> > totally fine. And it's confusing since at first glance the compiler >> > complaining about nested if and ambigous else doesn't make sense since >> > clearly there's only 1 if there. >> >> Ah, so the pattern the compiler tries to warn about is: >> >> if (foo) >> if (bar) >> /* stmts1 */ >> else >> /* stmts2 * >> >> Because it might not be immediately obvious with which if the else goes. >> Which is fair enough I suppose. >> >> OK, ACK. > > Just to bikeshed, there could be macros other than for_each_*() macros > that will want to use this internally, so perhaps it would be worth the > generic version being named something like if_noelse(). > > We could always add that as/when required, though. I think a better name would be really good, but both when we added it for i915 and when we move it to drm headers we drew a blank. if_noelse() describes pretty good what it does, but kinda fails on the "where should I use it" departement. If there's some consensus I can sed the patches quickly. -Daniel
On Mon, Jul 09, 2018 at 07:55:20PM +0200, Daniel Vetter wrote: > On Mon, Jul 9, 2018 at 6:12 PM, Mark Rutland <mark.rutland@arm.com> wrote: > > On Mon, Jul 09, 2018 at 06:03:42PM +0200, Peter Zijlstra wrote: > >> On Mon, Jul 09, 2018 at 05:52:04PM +0200, Daniel Vetter wrote: > >> > for_each_something(foo) > >> > if (foo->bla) > >> > call_bla(foo); > >> > else > >> > call_default(foo); > >> > > >> > Totally contrived, but this complains. Liberally sprinkling {} also shuts > >> > up the compiler, but it's a bit confusing given that a plain for {;;} is > >> > totally fine. And it's confusing since at first glance the compiler > >> > complaining about nested if and ambigous else doesn't make sense since > >> > clearly there's only 1 if there. > >> > >> Ah, so the pattern the compiler tries to warn about is: > >> > >> if (foo) > >> if (bar) > >> /* stmts1 */ > >> else > >> /* stmts2 * > >> > >> Because it might not be immediately obvious with which if the else goes. > >> Which is fair enough I suppose. > >> > >> OK, ACK. > > > > Just to bikeshed, there could be macros other than for_each_*() macros > > that will want to use this internally, so perhaps it would be worth the > > generic version being named something like if_noelse(). > > > > We could always add that as/when required, though. > > I think a better name would be really good, but both when we added it > for i915 and when we move it to drm headers we drew a blank. > if_noelse() describes pretty good what it does, but kinda fails on the > "where should I use it" departement. If there's some consensus I can > sed the patches quickly. Just to be clear: for_each_if() is fine by me, so no need to change things. Sorry for the noise! Mark.
diff --git a/include/linux/topology.h b/include/linux/topology.h index cb0775e1ee4b..4fba5a5b148d 100644 --- a/include/linux/topology.h +++ b/include/linux/topology.h @@ -40,7 +40,7 @@ #define for_each_node_with_cpus(node) \ for_each_online_node(node) \ - if (nr_cpus_node(node)) + for_each_if (nr_cpus_node(node)) int arch_update_cpu_topology(void);
Avoids complaints from gcc about ambiguous else clauses. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> --- include/linux/topology.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)