Edward Cree
2020-Nov-25 22:44 UTC
[Nouveau] [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
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