Miguel Ojeda
2020-Nov-26 14:53 UTC
[Nouveau] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, Nov 25, 2020 at 11:44 PM Edward Cree <ecree.xilinx at 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 meantWhat 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
Geert Uytterhoeven
2020-Nov-26 15:28 UTC
[Nouveau] [PATCH 000/141] Fix fall-through warnings for Clang
Hi Miguel, On Thu, Nov 26, 2020 at 3:54 PM Miguel Ojeda <miguel.ojeda.sandonis at gmail.com> wrote:> On Wed, Nov 25, 2020 at 11:44 PM Edward Cree <ecree.xilinx at 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.The maintainer is not necessarily the owner/author of the code, and thus may not know the intent of the code.> > 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...BTW, you cannot mindlessly fix the latter, as you cannot know if "(a == b)" or "((a = b))" was intended, without understanding the code (and the (possibly unavailable) data sheet, and the hardware, ...). P.S. So far I've stayed out of this thread, as I like it if the compiler flags possible mistakes. After all I was the one fixing new "may be used uninitialized" warnings thrown up by gcc-4.1, until (a bit later than) support for that compiler was removed... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds