Message-ID: <44005bde-f6d4-5eaa-39b8-1a5efeedb2d3@gmail.com>
Date:   Wed, 25 Nov 2020 22:44:35 +0000
From:   Edward Cree <ecree.xilinx@...il.com>
To:     Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
        James Bottomley <James.Bottomley@...senpartnership.com>
Cc:     Kees Cook <keescook@...omium.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        alsa-devel@...a-project.org, amd-gfx@...ts.freedesktop.org,
        bridge@...ts.linux-foundation.org, ceph-devel@...r.kernel.org,
        cluster-devel@...hat.com, coreteam@...filter.org,
        devel@...verdev.osuosl.org, dm-devel@...hat.com,
        drbd-dev@...ts.linbit.com, dri-devel@...ts.freedesktop.org,
        GR-everest-linux-l2@...vell.com, GR-Linux-NIC-Dev@...vell.com,
        intel-gfx@...ts.freedesktop.org, intel-wired-lan@...ts.osuosl.org,
        keyrings@...r.kernel.org, linux1394-devel@...ts.sourceforge.net,
        linux-acpi@...r.kernel.org, linux-afs@...ts.infradead.org,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-arm-msm@...r.kernel.org,
        linux-atm-general@...ts.sourceforge.net,
        linux-block@...r.kernel.org, linux-can@...r.kernel.org,
        linux-cifs@...r.kernel.org,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        linux-decnet-user@...ts.sourceforge.net,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        linux-fbdev@...r.kernel.org, linux-geode@...ts.infradead.org,
        linux-gpio@...r.kernel.org, linux-hams@...r.kernel.org,
        linux-hwmon@...r.kernel.org, linux-i3c@...ts.infradead.org,
        linux-ide@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-input <linux-input@...r.kernel.org>,
        linux-integrity@...r.kernel.org,
        linux-mediatek@...ts.infradead.org,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        linux-mmc@...r.kernel.org, Linux-MM <linux-mm@...ck.org>,
        linux-mtd@...ts.infradead.org, linux-nfs@...r.kernel.org,
        linux-rdma@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        linux-scsi@...r.kernel.org, linux-sctp@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com,
        linux-usb@...r.kernel.org, linux-watchdog@...r.kernel.org,
        linux-wireless <linux-wireless@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        netfilter-devel@...r.kernel.org, nouveau@...ts.freedesktop.org,
        op-tee@...ts.trustedfirmware.org, oss-drivers@...ronome.com,
        patches@...nsource.cirrus.com, rds-devel@....oracle.com,
        reiserfs-devel@...r.kernel.org, samba-technical@...ts.samba.org,
        selinux@...r.kernel.org, target-devel@...r.kernel.org,
        tipc-discussion@...ts.sourceforge.net,
        usb-storage@...ts.one-eyed-alien.net,
        virtualization@...ts.linux-foundation.org,
        wcn36xx@...ts.infradead.org,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        xen-devel@...ts.xenproject.org, linux-hardening@...r.kernel.org,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Nathan Chancellor <natechancellor@...il.com>,
        Miguel Ojeda <ojeda@...nel.org>, Joe Perches <joe@...ches.com>
Subject: Re: [PATCH 000/141] Fix fall-through warnings for Clang

On 25/11/2020 00:32, Miguel Ojeda wrote:
> I have said *authoring* lines of *this* kind takes a minute per line.
> Specifically: lines fixing the fallthrough warning mechanically and
> repeatedly where the compiler tells you to, and doing so full-time for
> a month.
<snip>
> It is useful since it makes intent clear.
To make the intent clear, you have to first be certain that you
 understand the intent; otherwise by adding either a break or a
 fallthrough to suppress the warning you are just destroying the
 information that "the intent of this code is unknown".
Figuring out the intent of a piece of unfamiliar code takes more
 than 1 minute; just because
    case foo:
        thing;
    case bar:
        break;
 produces identical code to
    case foo:
        thing;
        break;
    case bar:
        break;
 doesn't mean that *either* is correct — maybe the author meant
 to write
    case foo:
        return thing;
    case bar:
        break;
 and by inserting that break you've destroyed the marker that
 would direct someone who knew what the code was about to look
 at that point in the code and spot the problem.
Thus, you *always* have to look at more than just the immediate
 mechanical context of the code, to make a proper judgement that
 yes, this was the intent.  If you think that that sort of thing
 can be done in an *average* time of one minute, then I hope you
 stay away from code I'm responsible for!
One minute would be an optimistic target for code that, as the
 maintainer, one is already somewhat familiar with.  For code
 that you're seeing for the first time, as is usually the case
 with the people doing these mechanical fix-a-warning patches,
 it's completely unrealistic.

A warning is only useful because it makes you *think* about the
 code.  If you suppress the warning without doing that thinking,
 then you made the warning useless; and if the warning made you
 think about code that didn't *need* it, then the warning was
 useless from the start.

So make your mind up: does Clang's stricter -Wimplicit-fallthrough
 flag up code that needs thought (in which case the fixes take
 effort both to author and to review) or does it flag up code
 that can be mindlessly "fixed" (in which case the warning is
 worthless)?  Proponents in this thread seem to be trying to
 have it both ways.

-ed

Powered by blists - more mailing lists