Re: [PATCH 000/141] Fix fall-through warnings for Clang

Miguel Ojeda
2020-11-26T14:53:58Z

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