From: Randy Dunlap <rdunlap@infradead.org> To: NeilBrown <neilb@suse.com>, Andrew Morton <akpm@linux-foundation.org>, Daniel Vetter <daniel@ffwll.ch> Cc: LKML <linux-kernel@vger.kernel.org>, DRI Development <dri-devel@lists.freedesktop.org>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>, Gustavo Padovan <gustavo@padovan.org>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Sean Paul <seanpaul@chromium.org>, David Airlie <airlied@linux.ie>, Kees Cook <keescook@chromium.org>, Ingo Molnar <mingo@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Wei Wang <wvw@google.com>, Stefan Agner <stefan@agner.ch>, Andrei Vagin <avagin@openvz.org>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Yisheng Xie <ysxie@foxmail.com>, Peter Zijlstra <peterz@infradead.org>, Daniel Vetter <daniel.vetter@intel.com> Subject: Re: [PATCH] kernel.h: Add for_each_if() Date: Fri, 13 Jul 2018 16:42:59 -0700 [thread overview] Message-ID: <79133322-b04b-f005-f1f6-25c28c5058e4@infradead.org> (raw) In-Reply-To: <87bmbavhai.fsf@notabene.neil.brown.name> 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 :) -- ~Randy
WARNING: multiple messages have this Message-ID (diff)
From: Randy Dunlap <rdunlap@infradead.org> To: NeilBrown <neilb@suse.com>, Andrew Morton <akpm@linux-foundation.org>, Daniel Vetter <daniel@ffwll.ch> Cc: Kees Cook <keescook@chromium.org>, David Airlie <airlied@linux.ie>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>, LKML <linux-kernel@vger.kernel.org>, DRI Development <dri-devel@lists.freedesktop.org>, Yisheng Xie <ysxie@foxmail.com>, Peter Zijlstra <peterz@infradead.org>, Stefan Agner <stefan@agner.ch>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Wei Wang <wvw@google.com>, Daniel Vetter <daniel.vetter@intel.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Ingo Molnar <mingo@kernel.org>, Andrei Vagin <avagin@openvz.org> Subject: Re: [PATCH] kernel.h: Add for_each_if() Date: Fri, 13 Jul 2018 16:42:59 -0700 [thread overview] Message-ID: <79133322-b04b-f005-f1f6-25c28c5058e4@infradead.org> (raw) In-Reply-To: <87bmbavhai.fsf@notabene.neil.brown.name> 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 :) -- ~Randy _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2018-07-13 23:43 UTC|newest] Thread overview: 104+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-09 8:36 [PATCH 01/12] kernel.h: Add for_each_if() Daniel Vetter 2018-07-09 8:36 ` Daniel Vetter 2018-07-09 8:36 ` [PATCH 02/12] blk: use for_each_if Daniel Vetter 2018-07-09 8:36 ` Daniel Vetter 2018-07-11 16:40 ` Tejun Heo 2018-07-11 16:45 ` Tejun Heo 2018-07-11 18:30 ` Jens Axboe 2018-07-11 18:30 ` Jens Axboe 2018-07-11 18:50 ` Daniel Vetter 2018-07-11 19:31 ` Jens Axboe 2018-07-11 19:31 ` Jens Axboe 2018-07-11 20:06 ` Tejun Heo 2018-07-11 21:08 ` Daniel Vetter 2018-07-11 21:08 ` Daniel Vetter 2018-07-11 21:13 ` Jens Axboe 2018-07-11 21:13 ` Jens Axboe 2018-07-12 6:41 ` Daniel Vetter 2018-07-12 6:41 ` Daniel Vetter 2018-07-12 6:45 ` Joe Perches 2018-07-12 13:54 ` Jens Axboe 2018-07-12 15:32 ` Joe Perches 2018-07-12 15:32 ` Joe Perches 2018-07-13 9:28 ` Vlastimil Babka 2018-07-13 9:28 ` Vlastimil Babka 2018-07-09 8:36 ` [PATCH 03/12] cgroup: " Daniel Vetter 2018-07-09 8:36 ` Daniel Vetter 2018-07-11 16:46 ` Tejun Heo 2018-07-11 16:46 ` Tejun Heo 2018-07-09 8:36 ` [PATCH 04/12] cpufreq: " Daniel Vetter 2018-07-09 9:28 ` Eric Engestrom 2018-07-09 9:28 ` Eric Engestrom 2018-07-09 16:11 ` [PATCH] " Daniel Vetter 2018-07-09 16:11 ` Daniel Vetter 2018-07-09 21:36 ` Rafael J. Wysocki 2018-07-09 21:36 ` Rafael J. Wysocki 2018-07-09 8:36 ` [PATCH 05/12] dmar: Use for_each_If Daniel Vetter 2018-07-09 8:36 ` Daniel Vetter 2018-07-20 12:50 ` Joerg Roedel 2018-07-20 12:50 ` Joerg Roedel 2018-07-09 8:36 ` [PATCH 06/12] mm: use for_each_if Daniel Vetter 2018-07-09 8:36 ` Daniel Vetter 2018-07-09 18:00 ` Pavel Tatashin 2018-07-09 8:36 ` [PATCH 07/12] ide: " Daniel Vetter 2018-07-09 8:36 ` [PATCH 08/12] netdev: " Daniel Vetter 2018-07-09 8:36 ` Daniel Vetter 2018-07-09 8:36 ` [PATCH 09/12] nubus: " Daniel Vetter 2018-07-09 8:36 ` Daniel Vetter 2018-07-09 10:17 ` Finn Thain 2018-07-17 15:26 ` Geert Uytterhoeven 2018-07-09 8:36 ` [PATCH 10/12] pci: " Daniel Vetter 2018-07-09 8:36 ` Daniel Vetter 2018-07-09 22:48 ` Bjorn Helgaas 2018-07-09 22:48 ` Bjorn Helgaas 2018-07-09 8:36 ` [PATCH 11/12] sched: use for_each_if in topology.h Daniel Vetter 2018-07-09 8:36 ` Daniel Vetter 2018-07-09 10:36 ` Peter Zijlstra 2018-07-09 10:36 ` Peter Zijlstra 2018-07-09 15:00 ` Daniel Vetter 2018-07-09 15:00 ` Daniel Vetter 2018-07-09 15:12 ` Peter Zijlstra 2018-07-09 15:12 ` Peter Zijlstra 2018-07-09 15:52 ` Daniel Vetter 2018-07-09 15:52 ` Daniel Vetter 2018-07-09 16:03 ` Peter Zijlstra 2018-07-09 16:06 ` Daniel Vetter 2018-07-09 16:06 ` Daniel Vetter 2018-07-09 16:12 ` Mark Rutland 2018-07-09 17:55 ` [Intel-gfx] " Daniel Vetter 2018-07-09 17:55 ` Daniel Vetter 2018-07-11 16:51 ` [Intel-gfx] " Mark Rutland 2018-07-09 16:30 ` Peter Zijlstra 2018-07-09 16:30 ` Peter Zijlstra 2018-07-09 8:36 ` [PATCH 12/12] usb: use for_each_if Daniel Vetter 2018-07-09 8:36 ` Daniel Vetter 2018-07-09 8:36 ` [12/12] " Daniel Vetter 2018-07-09 8:50 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/12] kernel.h: Add for_each_if() Patchwork 2018-07-09 9:06 ` ✗ Fi.CI.BAT: failure " Patchwork 2018-07-09 11:50 ` [PATCH 01/12] " Andy Shevchenko 2018-07-09 11:50 ` Andy Shevchenko 2018-07-09 16:25 ` [PATCH] " Daniel Vetter 2018-07-09 16:25 ` Daniel Vetter 2018-07-09 18:30 ` Andy Shevchenko 2018-07-09 18:30 ` Andy Shevchenko 2018-07-09 23:30 ` Andrew Morton 2018-07-10 7:53 ` Daniel Vetter 2018-07-10 7:53 ` Daniel Vetter 2018-07-10 10:32 ` NeilBrown 2018-07-10 10:32 ` NeilBrown 2018-07-11 11:51 ` Daniel Vetter 2018-07-11 11:51 ` Daniel Vetter 2018-07-11 23:05 ` Andrew Morton 2018-07-11 23:05 ` Andrew Morton 2018-07-12 6:39 ` Daniel Vetter 2018-07-12 6:39 ` Daniel Vetter 2018-07-13 23:37 ` NeilBrown 2018-07-13 23:42 ` Randy Dunlap [this message] 2018-07-13 23:42 ` Randy Dunlap 2018-07-16 8:11 ` Andy Shevchenko 2018-07-16 15:41 ` Randy Dunlap 2018-07-16 15:41 ` Randy Dunlap 2018-07-16 22:16 ` NeilBrown 2018-07-09 16:50 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with kernel.h: Add for_each_if() (rev3) Patchwork 2018-07-09 17:05 ` ✓ Fi.CI.BAT: success " Patchwork 2018-07-10 2:08 ` ✓ Fi.CI.IGT: " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=79133322-b04b-f005-f1f6-25c28c5058e4@infradead.org \ --to=rdunlap@infradead.org \ --cc=airlied@linux.ie \ --cc=akpm@linux-foundation.org \ --cc=andriy.shevchenko@linux.intel.com \ --cc=avagin@openvz.org \ --cc=daniel.vetter@intel.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=gregkh@linuxfoundation.org \ --cc=gustavo@padovan.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=keescook@chromium.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maarten.lankhorst@linux.intel.com \ --cc=mingo@kernel.org \ --cc=neilb@suse.com \ --cc=peterz@infradead.org \ --cc=seanpaul@chromium.org \ --cc=stefan@agner.ch \ --cc=wvw@google.com \ --cc=ysxie@foxmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.