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.
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.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com 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 --- include/drm/drmP.h | 3 --- include/linux/kernel.h | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-)
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); \
Makes the macros resilient against if {} else {} blocks right afterwards.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tejun Heo tj@kernel.org Cc: Jens Axboe axboe@kernel.dk Cc: Shaohua Li shli@fb.com Cc: Kate Stewart kstewart@linuxfoundation.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Joseph Qi joseph.qi@linux.alibaba.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Arnd Bergmann arnd@arndb.de --- include/linux/blk-cgroup.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h index 6c666fd7de3c..f1c3afe42c26 100644 --- a/include/linux/blk-cgroup.h +++ b/include/linux/blk-cgroup.h @@ -382,7 +382,7 @@ static inline void blkg_put(struct blkcg_gq *blkg) */ #define blkg_for_each_descendant_pre(d_blkg, pos_css, p_blkg) \ css_for_each_descendant_pre((pos_css), &(p_blkg)->blkcg->css) \ - if (((d_blkg) = __blkg_lookup(css_to_blkcg(pos_css), \ + for_each_if (((d_blkg) = __blkg_lookup(css_to_blkcg(pos_css), \ (p_blkg)->q, false)))
/** @@ -397,7 +397,7 @@ static inline void blkg_put(struct blkcg_gq *blkg) */ #define blkg_for_each_descendant_post(d_blkg, pos_css, p_blkg) \ css_for_each_descendant_post((pos_css), &(p_blkg)->blkcg->css) \ - if (((d_blkg) = __blkg_lookup(css_to_blkcg(pos_css), \ + for_each_if (((d_blkg) = __blkg_lookup(css_to_blkcg(pos_css), \ (p_blkg)->q, false)))
/**
On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
Makes the macros resilient against if {} else {} blocks right afterwards.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tejun Heo tj@kernel.org Cc: Jens Axboe axboe@kernel.dk Cc: Shaohua Li shli@fb.com Cc: Kate Stewart kstewart@linuxfoundation.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Joseph Qi joseph.qi@linux.alibaba.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Arnd Bergmann arnd@arndb.de
Acked-by: Tejun Heo tj@kernel.org
Jens, it'd probably be best to route this through block tree.
Thanks.
On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
Makes the macros resilient against if {} else {} blocks right afterwards.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tejun Heo tj@kernel.org Cc: Jens Axboe axboe@kernel.dk Cc: Shaohua Li shli@fb.com Cc: Kate Stewart kstewart@linuxfoundation.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Joseph Qi joseph.qi@linux.alibaba.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Arnd Bergmann arnd@arndb.de
Acked-by: Tejun Heo tj@kernel.org
Jens, it'd probably be best to route this through block tree.
Oops, this requires an earlier patch to move the for_each_if def to a common header and should be routed together.
Thanks.
On 7/11/18 10:45 AM, Tejun Heo wrote:
On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
Makes the macros resilient against if {} else {} blocks right afterwards.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tejun Heo tj@kernel.org Cc: Jens Axboe axboe@kernel.dk Cc: Shaohua Li shli@fb.com Cc: Kate Stewart kstewart@linuxfoundation.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Joseph Qi joseph.qi@linux.alibaba.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Arnd Bergmann arnd@arndb.de
Acked-by: Tejun Heo tj@kernel.org
Jens, it'd probably be best to route this through block tree.
Oops, this requires an earlier patch to move the for_each_if def to a common header and should be routed together.
Yeah, this is a problem with the submission.
Always (ALWAYS) CC folks on at least the cover letter and generic earlier patches. Getting just one patch sent like this is mostly useless, and causes more harm than good.
On Wed, Jul 11, 2018 at 8:30 PM, Jens Axboe axboe@kernel.dk wrote:
On 7/11/18 10:45 AM, Tejun Heo wrote:
On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
Makes the macros resilient against if {} else {} blocks right afterwards.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tejun Heo tj@kernel.org Cc: Jens Axboe axboe@kernel.dk Cc: Shaohua Li shli@fb.com Cc: Kate Stewart kstewart@linuxfoundation.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Joseph Qi joseph.qi@linux.alibaba.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Arnd Bergmann arnd@arndb.de
Acked-by: Tejun Heo tj@kernel.org
Jens, it'd probably be best to route this through block tree.
Oops, this requires an earlier patch to move the for_each_if def to a common header and should be routed together.
Yeah, this is a problem with the submission.
Always (ALWAYS) CC folks on at least the cover letter and generic earlier patches. Getting just one patch sent like this is mostly useless, and causes more harm than good.
Ime sending a patch with more than 20 or so recipients means it gets stuck everywhere in moderation queues. Or outright spam filters. I thought the correct way to do this is to cc: mailing lists (lkml has them all), but apparently that's not how it's done. Despite that all the patch series I get never have the cover letter addressed to me either.
So what's the magic way to make this possible? -Daniel
On 7/11/18 12:50 PM, Daniel Vetter wrote:
On Wed, Jul 11, 2018 at 8:30 PM, Jens Axboe axboe@kernel.dk wrote:
On 7/11/18 10:45 AM, Tejun Heo wrote:
On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
Makes the macros resilient against if {} else {} blocks right afterwards.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tejun Heo tj@kernel.org Cc: Jens Axboe axboe@kernel.dk Cc: Shaohua Li shli@fb.com Cc: Kate Stewart kstewart@linuxfoundation.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Joseph Qi joseph.qi@linux.alibaba.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Arnd Bergmann arnd@arndb.de
Acked-by: Tejun Heo tj@kernel.org
Jens, it'd probably be best to route this through block tree.
Oops, this requires an earlier patch to move the for_each_if def to a common header and should be routed together.
Yeah, this is a problem with the submission.
Always (ALWAYS) CC folks on at least the cover letter and generic earlier patches. Getting just one patch sent like this is mostly useless, and causes more harm than good.
Ime sending a patch with more than 20 or so recipients means it gets stuck everywhere in moderation queues. Or outright spam filters. I thought the correct way to do this is to cc: mailing lists (lkml has them all), but apparently that's not how it's done. Despite that all the patch series I get never have the cover letter addressed to me either.
So what's the magic way to make this possible?
I don't think there's a git easy way of sending it out outside of just ensuring that everybody is CC'ed on everything. I don't mind that at all. I don't subscribe to lkml, and the patches weren't sent to linux-block. Hence all I see is this stand-alone patch, and logic would dictate that it's stand-alone (but it isn't).
On Wed, Jul 11, 2018 at 01:31:51PM -0600, Jens Axboe wrote:
I don't think there's a git easy way of sending it out outside of just ensuring that everybody is CC'ed on everything. I don't mind that at all. I don't subscribe to lkml, and the patches weren't sent to linux-block. Hence all I see is this stand-alone patch, and logic would dictate that it's stand-alone (but it isn't).
What I sometimes do is including a short blurb on each patch giving the overview and action hints (e.g. this is part of patchset doing XYZ and should be routed such and such). It's a bit redundant but has worked pretty well for patchsets with dependenat & sweeping changes.
Thanks.
On Wed, Jul 11, 2018 at 10:06 PM, Tejun Heo tj@kernel.org wrote:
On Wed, Jul 11, 2018 at 01:31:51PM -0600, Jens Axboe wrote:
I don't think there's a git easy way of sending it out outside of just ensuring that everybody is CC'ed on everything. I don't mind that at all. I don't subscribe to lkml, and the patches weren't sent to linux-block. Hence all I see is this stand-alone patch, and logic would dictate that it's stand-alone (but it isn't).
Hm yeah I forgot to add linux-block. But others where there's no dedicated list (or get_maintainers.pl didn't have one) also complained about not getting Cc'ed, and I can't Cc everyone for sweeping changes.
What I sometimes do is including a short blurb on each patch giving the overview and action hints (e.g. this is part of patchset doing XYZ and should be routed such and such). It's a bit redundant but has worked pretty well for patchsets with dependenat & sweeping changes.
Yeah I guess I can just copypaste/summarize patch 1 to all the subsequent patches, sounds like the best option. -Daniel
On 7/11/18 3:08 PM, Daniel Vetter wrote:
On Wed, Jul 11, 2018 at 10:06 PM, Tejun Heo tj@kernel.org wrote:
On Wed, Jul 11, 2018 at 01:31:51PM -0600, Jens Axboe wrote:
I don't think there's a git easy way of sending it out outside of just ensuring that everybody is CC'ed on everything. I don't mind that at all. I don't subscribe to lkml, and the patches weren't sent to linux-block. Hence all I see is this stand-alone patch, and logic would dictate that it's stand-alone (but it isn't).
Hm yeah I forgot to add linux-block. But others where there's no dedicated list (or get_maintainers.pl didn't have one) also complained about not getting Cc'ed, and I can't Cc everyone for sweeping changes.
I don't personally see a problem with just CC'ing everyone.
What I sometimes do is including a short blurb on each patch giving the overview and action hints (e.g. this is part of patchset doing XYZ and should be routed such and such). It's a bit redundant but has worked pretty well for patchsets with dependenat & sweeping changes.
Yeah I guess I can just copypaste/summarize patch 1 to all the subsequent patches, sounds like the best option.
Another approach might be to submit the first independent patch separately. Once that's in the kernel, you can send out the rest as independent patches instead of doing a cross-kernel series that all depend on one single patch. Seems to me that's where you run into issues, and it can be avoided quite easily.
On Wed, Jul 11, 2018 at 03:13:00PM -0600, Jens Axboe wrote:
On 7/11/18 3:08 PM, Daniel Vetter wrote:
On Wed, Jul 11, 2018 at 10:06 PM, Tejun Heo tj@kernel.org wrote:
On Wed, Jul 11, 2018 at 01:31:51PM -0600, Jens Axboe wrote:
I don't think there's a git easy way of sending it out outside of just ensuring that everybody is CC'ed on everything. I don't mind that at all. I don't subscribe to lkml, and the patches weren't sent to linux-block. Hence all I see is this stand-alone patch, and logic would dictate that it's stand-alone (but it isn't).
Hm yeah I forgot to add linux-block. But others where there's no dedicated list (or get_maintainers.pl didn't have one) also complained about not getting Cc'ed, and I can't Cc everyone for sweeping changes.
I don't personally see a problem with just CC'ing everyone.
What I sometimes do is including a short blurb on each patch giving the overview and action hints (e.g. this is part of patchset doing XYZ and should be routed such and such). It's a bit redundant but has worked pretty well for patchsets with dependenat & sweeping changes.
Yeah I guess I can just copypaste/summarize patch 1 to all the subsequent patches, sounds like the best option.
Another approach might be to submit the first independent patch separately. Once that's in the kernel, you can send out the rest as independent patches instead of doing a cross-kernel series that all depend on one single patch. Seems to me that's where you run into issues, and it can be avoided quite easily.
Well patch 1 died in a bikeshed (or is on live support at most). I kinda hoped that showing it's somewhat widely used pattern in the kernel would help it's cause, but alas not going to happen.
Anyway for next time around I'll crank up the Cc: knob to the max :-)
Thanks anyway for comments and stuff. -Daniel
On Wed, 2018-07-11 at 20:50 +0200, Daniel Vetter wrote:
On Wed, Jul 11, 2018 at 8:30 PM, Jens Axboe axboe@kernel.dk wrote:
On 7/11/18 10:45 AM, Tejun Heo wrote:
On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
Makes the macros resilient against if {} else {} blocks right afterwards.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tejun Heo tj@kernel.org Cc: Jens Axboe axboe@kernel.dk Cc: Shaohua Li shli@fb.com Cc: Kate Stewart kstewart@linuxfoundation.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Joseph Qi joseph.qi@linux.alibaba.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Arnd Bergmann arnd@arndb.de
Acked-by: Tejun Heo tj@kernel.org
Jens, it'd probably be best to route this through block tree.
Oops, this requires an earlier patch to move the for_each_if def to a common header and should be routed together.
Yeah, this is a problem with the submission.
Always (ALWAYS) CC folks on at least the cover letter and generic earlier patches. Getting just one patch sent like this is mostly useless, and causes more harm than good.
Ime sending a patch with more than 20 or so recipients means it gets stuck everywhere in moderation queues. Or outright spam filters. I thought the correct way to do this is to cc: mailing lists (lkml has them all), but apparently that's not how it's done. Despite that all the patch series I get never have the cover letter addressed to me either.
So what's the magic way to make this possible?
Jens' advice is crap.
There is no generic way to make this possible.
BCC's don't work, series that touch multiple subsystems get rejected when the recipient list is too large.
I think you did it correctly.
On 7/12/18 12:45 AM, Joe Perches wrote:
On Wed, 2018-07-11 at 20:50 +0200, Daniel Vetter wrote:
On Wed, Jul 11, 2018 at 8:30 PM, Jens Axboe axboe@kernel.dk wrote:
On 7/11/18 10:45 AM, Tejun Heo wrote:
On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
Makes the macros resilient against if {} else {} blocks right afterwards.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tejun Heo tj@kernel.org Cc: Jens Axboe axboe@kernel.dk Cc: Shaohua Li shli@fb.com Cc: Kate Stewart kstewart@linuxfoundation.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Joseph Qi joseph.qi@linux.alibaba.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Arnd Bergmann arnd@arndb.de
Acked-by: Tejun Heo tj@kernel.org
Jens, it'd probably be best to route this through block tree.
Oops, this requires an earlier patch to move the for_each_if def to a common header and should be routed together.
Yeah, this is a problem with the submission.
Always (ALWAYS) CC folks on at least the cover letter and generic earlier patches. Getting just one patch sent like this is mostly useless, and causes more harm than good.
Ime sending a patch with more than 20 or so recipients means it gets stuck everywhere in moderation queues. Or outright spam filters. I thought the correct way to do this is to cc: mailing lists (lkml has them all), but apparently that's not how it's done. Despite that all the patch series I get never have the cover letter addressed to me either.
So what's the magic way to make this possible?
Jens' advice is crap.
There is no generic way to make this possible.
Nobody claimed there was. And the advice is perfectly fine, sending out patches to folks that have hidden dependencies on other patches is a no-go.
BCC's don't work, series that touch multiple subsystems get rejected when the recipient list is too large.
I think you did it correctly.
Clearly that's not the case, regardless of what you think.
Thanks for your invaluable and useful feedback, sharing your vast experience in patchsets with dependencies.
On Thu, 2018-07-12 at 07:54 -0600, Jens Axboe wrote:
Thanks for your invaluable and useful feedback, sharing your vast experience in patchsets with dependencies.
I've probably more experience sending patchsets with dependencies across subsystems than anyone.
There is no single style that works and I've probably tried them all.
It's actually a somewhat significant issue within this community that could use some arbitration.
On 07/12/2018 08:45 AM, Joe Perches wrote:
On Wed, 2018-07-11 at 20:50 +0200, Daniel Vetter wrote:
On Wed, Jul 11, 2018 at 8:30 PM, Jens Axboe axboe@kernel.dk wrote:
On 7/11/18 10:45 AM, Tejun Heo wrote:
On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
Makes the macros resilient against if {} else {} blocks right afterwards.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tejun Heo tj@kernel.org Cc: Jens Axboe axboe@kernel.dk Cc: Shaohua Li shli@fb.com Cc: Kate Stewart kstewart@linuxfoundation.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Joseph Qi joseph.qi@linux.alibaba.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Arnd Bergmann arnd@arndb.de
Acked-by: Tejun Heo tj@kernel.org
Jens, it'd probably be best to route this through block tree.
Oops, this requires an earlier patch to move the for_each_if def to a common header and should be routed together.
Yeah, this is a problem with the submission.
Always (ALWAYS) CC folks on at least the cover letter and generic earlier patches. Getting just one patch sent like this is mostly useless, and causes more harm than good.
Ime sending a patch with more than 20 or so recipients means it gets stuck everywhere in moderation queues. Or outright spam filters. I thought the correct way to do this is to cc: mailing lists (lkml has them all), but apparently that's not how it's done. Despite that all the patch series I get never have the cover letter addressed to me either.
So what's the magic way to make this possible?
Jens' advice is crap.
This statement was rather offensive and totally uncalled for, AFAICS. Why did you write it like that?
There is no generic way to make this possible.
BCC's don't work, series that touch multiple subsystems get rejected when the recipient list is too large.
I don't know what's the usual limit for recipient list, probably never hit it myself, but for series that are not so large, I use this approach to make sure the cover letter is CC'd to everyone that's CC'd in any patch in the series:
- add per-patch Cc:'s to the git commit logs - clear out *.patch from the working dir - git format-patch --cover-letter ... - edit cover letter - git send-email ... --cc-cmd=./cc.sh ...
where cc.sh contains this:
#/bin/sh if [[ $1 == *cover-letter* ]]; then grep '<.*@.*>' -h *.patch | sed 's/^.*: //' | sort | uniq else grep '<.*@.*>' -h $1 | sed 's/^.*: //' | sort | uniq fi
That proceses all tags besides Cc (acked-by, reported-by etc) and turns them to Cc's for each patch (or does git now do that by itself as well?) and for cover letter, it accumulates that from all the patches.
Vlastimil
I think you did it correctly.
Avoids the need to invert the condition instead of the open-coded version.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tejun Heo tj@kernel.org Cc: Li Zefan lizefan@huawei.com Cc: Johannes Weiner hannes@cmpxchg.org Cc: cgroups@vger.kernel.org --- include/linux/cgroup.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h index c9fdf6f57913..666c6200d61d 100644 --- a/include/linux/cgroup.h +++ b/include/linux/cgroup.h @@ -287,9 +287,7 @@ void css_task_iter_end(struct css_task_iter *it); for ((leader) = cgroup_taskset_first((tset), &(dst_css)); \ (leader); \ (leader) = cgroup_taskset_next((tset), &(dst_css))) \ - if ((leader) != (leader)->group_leader) \ - ; \ - else + for_each_if ((leader) == (leader)->group_leader)
/* * Inline functions.
On Mon, Jul 09, 2018 at 10:36:41AM +0200, Daniel Vetter wrote:
Avoids the need to invert the condition instead of the open-coded version.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tejun Heo tj@kernel.org Cc: Li Zefan lizefan@huawei.com Cc: Johannes Weiner hannes@cmpxchg.org Cc: cgroups@vger.kernel.org
Acked-by: Tejun Heo tj@kernel.org
Please feel free to route as you see fit.
Thanks.
Avoids the inverted condition compared to the open coded version.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: "Rafael J. Wysocki" rjw@rjwysocki.net Cc: Viresh Kumar viresh.kumar@linaro.org Cc: linux-pm@vger.kernel.org --- include/linux/cpufreq.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 882a9b9e34bc..f2028c229b96 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -649,9 +649,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
#define cpufreq_for_each_valid_entry(pos, table) \ for (pos = table; pos->frequency != CPUFREQ_TABLE_END; pos++) \ - if (pos->frequency == CPUFREQ_ENTRY_INVALID) \ - continue; \ - else + for_each_if (pos->frequency != CPUFREQ_ENTRY_INVALID)
/* * cpufreq_for_each_valid_entry_idx - iterate with index over a cpufreq @@ -663,9 +661,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
#define cpufreq_for_each_valid_entry_idx(pos, table, idx) \ cpufreq_for_each_entry_idx(pos, table, idx) \ - if (pos->frequency == CPUFREQ_ENTRY_INVALID) \ - continue; \ - else + for_each_if (pos->frequency == CPUFREQ_ENTRY_INVALID)
int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
On Monday, 2018-07-09 10:36:42 +0200, Daniel Vetter wrote:
Avoids the inverted condition compared to the open coded version.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: "Rafael J. Wysocki" rjw@rjwysocki.net Cc: Viresh Kumar viresh.kumar@linaro.org Cc: linux-pm@vger.kernel.org
include/linux/cpufreq.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 882a9b9e34bc..f2028c229b96 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -649,9 +649,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
#define cpufreq_for_each_valid_entry(pos, table) \ for (pos = table; pos->frequency != CPUFREQ_TABLE_END; pos++) \
if (pos->frequency == CPUFREQ_ENTRY_INVALID) \continue; \else
for_each_if (pos->frequency != CPUFREQ_ENTRY_INVALID)/*
- cpufreq_for_each_valid_entry_idx - iterate with index over a cpufreq
@@ -663,9 +661,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
#define cpufreq_for_each_valid_entry_idx(pos, table, idx) \ cpufreq_for_each_entry_idx(pos, table, idx) \
if (pos->frequency == CPUFREQ_ENTRY_INVALID) \continue; \else
for_each_if (pos->frequency == CPUFREQ_ENTRY_INVALID)
Should be `!=` there ^
int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
2.18.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Avoids the inverted condition compared to the open coded version.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: "Rafael J. Wysocki" rjw@rjwysocki.net Cc: Viresh Kumar viresh.kumar@linaro.org Cc: linux-pm@vger.kernel.org Cc: Eric Engestrom eric.engestrom@intel.com -- v2: Fix the logic fumble in the 2nd hunk, spotted by Eric. --- include/linux/cpufreq.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 882a9b9e34bc..3a774f523d00 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -649,9 +649,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
#define cpufreq_for_each_valid_entry(pos, table) \ for (pos = table; pos->frequency != CPUFREQ_TABLE_END; pos++) \ - if (pos->frequency == CPUFREQ_ENTRY_INVALID) \ - continue; \ - else + for_each_if (pos->frequency != CPUFREQ_ENTRY_INVALID)
/* * cpufreq_for_each_valid_entry_idx - iterate with index over a cpufreq @@ -663,9 +661,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
#define cpufreq_for_each_valid_entry_idx(pos, table, idx) \ cpufreq_for_each_entry_idx(pos, table, idx) \ - if (pos->frequency == CPUFREQ_ENTRY_INVALID) \ - continue; \ - else + for_each_if (pos->frequency != CPUFREQ_ENTRY_INVALID)
int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
On Mon, Jul 9, 2018 at 6:11 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Avoids the inverted condition compared to the open coded version.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: "Rafael J. Wysocki" rjw@rjwysocki.net Cc: Viresh Kumar viresh.kumar@linaro.org Cc: linux-pm@vger.kernel.org Cc: Eric Engestrom eric.engestrom@intel.com -- v2: Fix the logic fumble in the 2nd hunk, spotted by Eric.
Acked-by: Rafael J. Wysocki rafael.j.wysocki@intel.com
Or do you want me to apply it?
include/linux/cpufreq.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 882a9b9e34bc..3a774f523d00 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -649,9 +649,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
#define cpufreq_for_each_valid_entry(pos, table) \ for (pos = table; pos->frequency != CPUFREQ_TABLE_END; pos++) \
if (pos->frequency == CPUFREQ_ENTRY_INVALID) \continue; \else
for_each_if (pos->frequency != CPUFREQ_ENTRY_INVALID)/*
- cpufreq_for_each_valid_entry_idx - iterate with index over a cpufreq
@@ -663,9 +661,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
#define cpufreq_for_each_valid_entry_idx(pos, table, idx) \ cpufreq_for_each_entry_idx(pos, table, idx) \
if (pos->frequency == CPUFREQ_ENTRY_INVALID) \continue; \else
for_each_if (pos->frequency != CPUFREQ_ENTRY_INVALID)int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
2.18.0
Avoids the inverted conditions compared to the open coded version.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Joerg Roedel jroedel@suse.de --- include/linux/dmar.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/include/linux/dmar.h b/include/linux/dmar.h index e2433bc50210..99397504e75e 100644 --- a/include/linux/dmar.h +++ b/include/linux/dmar.h @@ -85,15 +85,15 @@ extern struct list_head dmar_drhd_units;
#define for_each_active_drhd_unit(drhd) \ list_for_each_entry_rcu(drhd, &dmar_drhd_units, list) \ - if (drhd->ignored) {} else + for_each_if (!drhd->ignored)
#define for_each_active_iommu(i, drhd) \ list_for_each_entry_rcu(drhd, &dmar_drhd_units, list) \ - if (i=drhd->iommu, drhd->ignored) {} else + for_each_if ((i=drhd->iommu, !drhd->ignored))
#define for_each_iommu(i, drhd) \ list_for_each_entry_rcu(drhd, &dmar_drhd_units, list) \ - if (i=drhd->iommu, 0) {} else + for_each_if ((i=drhd->iommu, true))
static inline bool dmar_rcu_check(void) { @@ -108,7 +108,8 @@ static inline bool dmar_rcu_check(void) NULL, (p) < (c)); (p)++)
#define for_each_active_dev_scope(a, c, p, d) \ - for_each_dev_scope((a), (c), (p), (d)) if (!(d)) { continue; } else + for_each_dev_scope((a), (c), (p), (d)) \ + for_each_if (d)
extern int dmar_table_init(void); extern int dmar_dev_scope_init(void);
On Mon, Jul 09, 2018 at 10:36:43AM +0200, Daniel Vetter wrote:
#define for_each_active_drhd_unit(drhd) \ list_for_each_entry_rcu(drhd, &dmar_drhd_units, list) \
if (drhd->ignored) {} else
for_each_if (!drhd->ignored)
Hmm, in my tree the macro comes from
include/drm/drmP.h:#define for_each_if(condition) if (!(condition)) {} else
Please re-submit when the macro is in a generic header upstream.
Joerg
Avoids the inverted condition of the open-coded version.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Andrew Morton akpm@linux-foundation.org Cc: Michal Hocko mhocko@suse.com Cc: Vlastimil Babka vbabka@suse.cz Cc: Mel Gorman mgorman@techsingularity.net Cc: David Rientjes rientjes@google.com Cc: Kemi Wang kemi.wang@intel.com Cc: Pavel Tatashin pasha.tatashin@oracle.com Cc: Petr Tesarik ptesarik@suse.com Cc: YASUAKI ISHIMATSU yasu.isimatu@gmail.com Cc: Andrey Ryabinin aryabinin@virtuozzo.com Cc: Nikolay Borisov nborisov@suse.com Cc: linux-mm@kvack.org --- include/linux/mmzone.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h index 32699b2dc52a..1bd5f4c72c8b 100644 --- a/include/linux/mmzone.h +++ b/include/linux/mmzone.h @@ -940,9 +940,7 @@ extern struct zone *next_zone(struct zone *zone); for (zone = (first_online_pgdat())->node_zones; \ zone; \ zone = next_zone(zone)) \ - if (!populated_zone(zone)) \ - ; /* do nothing */ \ - else + for_each_if (populated_zone(zone))
static inline struct zone *zonelist_zone(struct zoneref *zoneref) {
LGTM:
Reviewed-by: Pavel Tatashin pasha.tatashin@oracle.com
Avoids complaints from gcc about ambigious else clauses. Not that any of those are likely to show up in ide ...
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: "David S. Miller" davem@davemloft.net Cc: linux-ide@vger.kernel.org --- include/linux/ide.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/ide.h b/include/linux/ide.h index c74b0321922a..1530d81319ef 100644 --- a/include/linux/ide.h +++ b/include/linux/ide.h @@ -1601,7 +1601,7 @@ static inline void ide_set_drivedata(ide_drive_t *drive, void *data)
#define ide_port_for_each_present_dev(i, dev, port) \ for ((i) = 0; ((dev) = (port)->devices[i]) || (i) < MAX_DRIVES; (i)++) \ - if ((dev)->dev_flags & IDE_DFLAG_PRESENT) + for_each_if ((dev)->dev_flags & IDE_DFLAG_PRESENT)
#define ide_host_for_each_port(i, port, host) \ for ((i) = 0; ((port) = (host)->ports[i]) || (i) < MAX_HOST_PORTS; (i)++)
Avoids complaints from gcc about ambiguous else clauses.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: "David S. Miller" davem@davemloft.net Cc: netdev@vger.kernel.org --- include/linux/netdevice.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 3ec9850c7936..cdffe7f88dbe 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -2476,7 +2476,7 @@ extern rwlock_t dev_base_lock; /* Device list lock */ list_for_each_entry_continue_rcu(d, &(net)->dev_base_head, dev_list) #define for_each_netdev_in_bond_rcu(bond, slave) \ for_each_netdev_rcu(&init_net, slave) \ - if (netdev_master_upper_dev_get_rcu(slave) == (bond)) + for_each_if (netdev_master_upper_dev_get_rcu(slave) == (bond)) #define net_device_entry(lh) list_entry(lh, struct net_device, dev_list)
static inline struct net_device *next_net_device(struct net_device *dev)
Avoids the inverted check compared to the open-coded version.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Finn Thain fthain@telegraphics.com.au Cc: linux-m68k@lists.linux-m68k.org --- include/linux/nubus.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/nubus.h b/include/linux/nubus.h index eba50b057f6f..17fd07578ef7 100644 --- a/include/linux/nubus.h +++ b/include/linux/nubus.h @@ -127,7 +127,7 @@ struct nubus_rsrc *nubus_next_rsrc_or_null(struct nubus_rsrc *from); for (f = nubus_first_rsrc_or_null(); f; f = nubus_next_rsrc_or_null(f))
#define for_each_board_func_rsrc(b, f) \ - for_each_func_rsrc(f) if (f->board != b) {} else + for_each_func_rsrc(f) for_each_if (f->board == b)
/* These are somewhat more NuBus-specific. They all return 0 for success and -1 for failure, as you'd expect. */
On Mon, 9 Jul 2018, Daniel Vetter wrote:
Avoids the inverted check compared to the open-coded version.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Finn Thain fthain@telegraphics.com.au Cc: linux-m68k@lists.linux-m68k.org
Acked-by: Finn Thain fthain@telegraphics.com.au
include/linux/nubus.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/nubus.h b/include/linux/nubus.h index eba50b057f6f..17fd07578ef7 100644 --- a/include/linux/nubus.h +++ b/include/linux/nubus.h @@ -127,7 +127,7 @@ struct nubus_rsrc *nubus_next_rsrc_or_null(struct nubus_rsrc *from); for (f = nubus_first_rsrc_or_null(); f; f = nubus_next_rsrc_or_null(f))
#define for_each_board_func_rsrc(b, f) \
- for_each_func_rsrc(f) if (f->board != b) {} else
- for_each_func_rsrc(f) for_each_if (f->board == b)
/* These are somewhat more NuBus-specific. They all return 0 for success and -1 for failure, as you'd expect. */
Hi Daniel,
On Mon, Jul 9, 2018 at 10:44 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:
Avoids the inverted check compared to the open-coded version.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Finn Thain fthain@telegraphics.com.au Cc: linux-m68k@lists.linux-m68k.org
Thanks for your patch!
--- a/include/linux/nubus.h +++ b/include/linux/nubus.h @@ -127,7 +127,7 @@ struct nubus_rsrc *nubus_next_rsrc_or_null(struct nubus_rsrc *from); for (f = nubus_first_rsrc_or_null(); f; f = nubus_next_rsrc_or_null(f))
#define for_each_board_func_rsrc(b, f) \
for_each_func_rsrc(f) if (f->board != b) {} else
for_each_func_rsrc(f) for_each_if (f->board == b)
drivers/net/ethernet/8390/mac8390.c: In function ‘mac8390_device_probe’: drivers/net/ethernet/8390/mac8390.c:402: error: implicit declaration of function ‘for_each_if’ drivers/net/ethernet/8390/mac8390.c:402: error: expected ‘;’ before ‘{’ token
Apparently this depends on patch [01/12], which wasn't CCed to linux-m68k@lists.linux-m68k.org?
Please don't split CCs if all patches in the a series are not independent. Thanks!
Gr{oetje,eeting}s,
Geert
Avoids the inverted condition compared to the open-coded version.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Bjorn Helgaas bhelgaas@google.com Cc: linux-pci@vger.kernel.org --- include/linux/pci.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/pci.h b/include/linux/pci.h index 340029b2fb38..1484471ed048 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -601,7 +601,7 @@ static inline bool pci_is_bridge(struct pci_dev *dev)
#define for_each_pci_bridge(dev, bus) \ list_for_each_entry(dev, &bus->devices, bus_list) \ - if (!pci_is_bridge(dev)) {} else + for_each_if (pci_is_bridge(dev))
static inline struct pci_dev *pci_upstream_bridge(struct pci_dev *dev) {
On Mon, Jul 09, 2018 at 10:36:48AM +0200, Daniel Vetter wrote:
Avoids the inverted condition compared to the open-coded version.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Bjorn Helgaas bhelgaas@google.com Cc: linux-pci@vger.kernel.org
Acked-by: Bjorn Helgaas bhelgaas@google.com
I assume you'll merge this with the rest of the series.
include/linux/pci.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/pci.h b/include/linux/pci.h index 340029b2fb38..1484471ed048 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -601,7 +601,7 @@ static inline bool pci_is_bridge(struct pci_dev *dev)
#define for_each_pci_bridge(dev, bus) \ list_for_each_entry(dev, &bus->devices, bus_list) \
if (!pci_is_bridge(dev)) {} else
for_each_if (pci_is_bridge(dev))static inline struct pci_dev *pci_upstream_bridge(struct pci_dev *dev) { -- 2.18.0
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(-)
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);
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:
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:
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 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.
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(); }
Avoids the inverted condition compared to the open-coded version.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: linux-usb@vger.kernel.org --- include/linux/usb.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/usb.h b/include/linux/usb.h index 4cdd515a4385..a9da7237c619 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -732,7 +732,7 @@ extern struct usb_device *usb_hub_find_child(struct usb_device *hdev, for (port1 = 1, child = usb_hub_find_child(hdev, port1); \ port1 <= hdev->maxchild; \ child = usb_hub_find_child(hdev, ++port1)) \ - if (!child) continue; else + for_each_if (child)
/* USB device locking */ #define usb_lock_device(udev) device_lock(&(udev)->dev)
On Mon, 2018-07-09 at 10:36 +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.
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.
Thanks, Daniel!
Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
Signed-off-by: Daniel Vetter daniel.vetter@intel.com 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
include/drm/drmP.h | 3 --- include/linux/kernel.h | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-)
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(-)
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); \
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
dri-devel@lists.freedesktop.org
-
Andrew Morton -
Andy Shevchenko -
Bjorn Helgaas -
Daniel Vetter -
Daniel Vetter -
Eric Engestrom -
Finn Thain -
Geert Uytterhoeven -
Jens Axboe -
Joe Perches -
Joerg Roedel -
Mark Rutland -
NeilBrown -
Pavel Tatashin -
Peter Zijlstra -
Rafael J. Wysocki -
Randy Dunlap -
Tejun Heo -
Vlastimil Babka