lkml.org 
[lkml]   [2020]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 000/141] Fix fall-through warnings for Clang
    On Wed, Nov 25, 2020 at 11:44 PM Edward Cree <ecree.xilinx@gmail.com> wrote:
    >
    > 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".

    If you don't know what the intent of your own code is, then you
    *already* have a problem in your hands.

    > 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

    What takes 1 minute is adding it *mechanically* by the author, i.e. so
    that you later compare whether codegen is the same.

    > 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.

    Then it means you already have a bug. This patchset gives the
    maintainer a chance to notice it, which is a good thing. The "you've
    destroyed the market" claim is bogus, because:
    1. you were not looking into it
    2. you didn't notice the bug so far
    3. is implicit -- harder to spot
    4. is only useful if you explicitly take a look at this kind of bug.
    So why don't you do it now?

    > 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.

    I find that is the responsibility of the maintainers and reviewers for
    tree-wide patches like this, assuming they want. They can also keep
    the behavior (and the bugs) without spending time. Their choice.

    > 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!

    Please don't accuse others of recklessness or incompetence, especially
    if you didn't understand what they said.

    > 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.

    We are not suppressing the warning. Quite the opposite, in fact.

    > 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)

    As I said several times already, it does take time to review if the
    maintainer wants to take the chance to see if they had a bug to begin
    with, but it does not require thought for the author if they just go
    for equivalent codegen.

    > 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.

    A warning is not worthless just because you can mindlessly fix it.
    There are many counterexamples, e.g. many
    checkpatch/lint/lang-format/indentation warnings, functional ones like
    the `if (a = b)` warning...

    Cheers,
    Miguel

    \
     
     \ /
      Last update: 2020-11-26 15:56    [W:3.794 / U:0.368 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site