Re: [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
Hi Miguel,
On Thu, Nov 26, 2020 at 3:54 PM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
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.
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
On Thu, Nov 26, 2020 at 4:28 PM Geert Uytterhoeven geert@linux-m68k.org wrote:
Hi Miguel,
On Thu, Nov 26, 2020 at 3:54 PM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
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.
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, ...).
to allow assignments in if statements was clearly a mistake and if you need outside information to understand the code, your code is the issue already.
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@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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Nov 26, 2020 at 4:28 PM Geert Uytterhoeven geert@linux-m68k.org wrote:
The maintainer is not necessarily the owner/author of the code, and thus may not know the intent of the code.
Agreed, I was not blaming maintainers -- just trying to point out that the problem is there :-)
In those cases, it is still very useful: we add the `fallthrough` and a comment saying `FIXME: fallthrough intended? Figure this out...`. Thus a previous unknown unknown is now a known unknown. And no new unknown unknowns will be introduced since we enabled the warning globally.
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, ...).
That's right, I was referring to the cases where the compiler saves someone time from a typo they just made.
Cheers, Miguel
participants (3)
-
Geert Uytterhoeven -
Karol Herbst -
Miguel Ojeda