I agree with one of the comments there. There’s a difference between dealing with lots of warnings suddenly and keeping the warnings at 0. The second one is much easier and gives next to no overhead when dealing with code. Maybe you’ll need an annotation here and there, maybe you’ll need to dismiss in the UI like in the example from the post. But it’s really not that much of an issue if you deal with everything before the code is even merged.
So yes, please give me warnings turned up to 11 on my PRs and I’ll make sure none of those ever bother other people. It also works around another complaint - that code was never working / changed. It gets fixed before the first deployment.
I’ve found with most tools, as others have noted, if you start with warnings cranked up to 11 from day 1 and fix every warning you see, it’s not actually much of a drain on productivity. I’ve used a few though that really were over the top.
The most egregious of these was pylint (disclaimer: it’s been several years since I’ve done much python work, so it’s possible the situation has improved). Many of the warnings are highly subjective, questions of style, represent questionable practice to begin with, or just don’t work very well. When I’ve tried to go all-in on pylint for a new project, I’ve quickly built up a list of warnings I’ve ended up turning off. Some things I shut off for the last python project I was working on:
Warn when a warning is disabled locally.
Warn when a class has too few public methods
Warn when there’s a FIXME comment
A bunch of things that were necessary to write code that was portable between python 2 and 3 (back when 2 was still supported).
A bunch of style things that sound like they might be good guidelines until you try to use them and find that when they do trigger fixing them almost always makes the code worse
Some style things that conflict with warnings from other linters.
Things that seem like an obvious win, but have so many false positives they’re not worth it. E.g. accessing a non-existent field, which really, really doesn’t like code that uses __getattr__ and friends.
I could go on and on. It seems like the bar for merging a useful warning was just unfathomably low.
I’ve found myself agreeing with this sentiment more and more as I’ve gotten older. Especially this part:
Modifying code to fix a non-issue has a non-zero chance of introducing a real bug. If you have code that has been running in production for a long time, without error… trying to “fix it” (when it is not broken) may actually break it. You should be conservative about fixing code without strong evidence that there is a real issue.
That was burned into my thought process by this incident.
That depends a bit on the kind of fix. If it’s simply adding no lint comments then it can’t introduce a bug, only hide one from static analysers. For example, in CHERIoT we run clang-tidy to check a bunch of things and the most common false positive is using macro arguments without wrapping them in brackets. The warning isn’t clever enough to understand that some arguments are type names and so putting them in brackets would break and we add no-lint comments on these. This is also a hint to a future code reviewer that someone has thought about this use.
The warning from the example is weird though. In C, a T* might be a pointer to a T or to an array of T, so a lot of uses of a T* should expect to handle more than sizeof(T) data. I can’t imagine it not giving a load of false positives. That said, a coding style that doesn’t let a static analyser (or reader of the code) understand the bounds of a pointee makes me wince somewhat.
Deciding whether a warning should be suppressed can also take quite a bit of time to investigate, assuming a good-faith effort is done. This can be very fatiguing when you have to do this over and over, and also to keep suppressing the same warning every single time is just a hassle too. Disabling it project-wide might not be a good idea since not every instance will be a false positive.
Honestly this just reads as an argument for why static analysis in C is doomed to failure due to poor language design choices. In modern languages such as TypeScript, Rust, etc., I tend to pick the strictest static analysis settings for greenfield projects and have found 0 downsides - linting legacy code is a slightly different matter and some compromises must be taken in that case, but it just seems like for C you might as well not bother.
The author cropped out most of the code in that screenshot so it’s not really possible to understand the warning, but it seems uselessly broad. The nature of C is that you’re often casting pointers.
I’ve never used CodeQL, but the Clang static analyzer is pretty good; in medium-size C++ codebases I work on it might find one bogus issue per 10K LOC. often those can be silenced by adding an assertion, when the analyzer finds an issue on a code path that can’t actually occur.
The nature of C (and C++) is that warnings are essential; there is so much clearly-wrong code that’s allowed by the language spec, mostly for historical reasons. The first thing I do in any new project is crank up warnings to, yes, 11 (-Wpedantic -Werror).
I hope that it is not a symptom of a larger trend where programming becomes bureaucratic.
The trend of tools over skills/experience certainly speaks to that. I think we’re at the peak of the “tools will save the industry!” belief. They help quite a bit but they can’t make a junior dev into a senior one.
Will be interesting to see if the “tools > everything else” belief holds up once AI-written code has proliferated. Will we grant AI slack because it’s not human? Or require AIs to only write code that conforms to all our tools? Who will fix the AI generated code so it passes all the tooling checks?
if you have warnings that you don’t deal with, there is a much greater chance that real issues will drown in the sea of noise.
but having a zero tolerance policy on warnings can be equally problematic. As well as the time it takes, as mentioned in the post, there are other noise effects it introduces. For example, silencing linter often involves adding comments to code, making your code noisier. It also means you are likely to choose colour themes in comments are made low contrast, which is the opposite of what you want (despite being done everywhere), because it leads to the genuinely important stuff being ignored.
One of the things I found amazing working with hardware folks was that they just expect Verilog to spit out tens of thousands of warnings. I learned this when we had a problem that the reset line on the network interface was wired high and so the network didn’t work. It turned out that the Verilog compiler had been warning that it was in an undefined state for ages, but it connects undefined things to whatever happens to be most convenient and that was low until another change made it high. The previous bug was that it was impossible to reset the NIC, but we never did that without reprogramming the FPGA anyway. The new one was that the NIC never came out of reset. The compiler clearly warned about the problem, but it also warned about thousands of things that were not important and so no one even bothered reading the warning.
I agree with one of the comments there. There’s a difference between dealing with lots of warnings suddenly and keeping the warnings at 0. The second one is much easier and gives next to no overhead when dealing with code. Maybe you’ll need an annotation here and there, maybe you’ll need to dismiss in the UI like in the example from the post. But it’s really not that much of an issue if you deal with everything before the code is even merged.
So yes, please give me warnings turned up to 11 on my PRs and I’ll make sure none of those ever bother other people. It also works around another complaint - that code was never working / changed. It gets fixed before the first deployment.
I’ve found with most tools, as others have noted, if you start with warnings cranked up to 11 from day 1 and fix every warning you see, it’s not actually much of a drain on productivity. I’ve used a few though that really were over the top.
The most egregious of these was pylint (disclaimer: it’s been several years since I’ve done much python work, so it’s possible the situation has improved). Many of the warnings are highly subjective, questions of style, represent questionable practice to begin with, or just don’t work very well. When I’ve tried to go all-in on pylint for a new project, I’ve quickly built up a list of warnings I’ve ended up turning off. Some things I shut off for the last python project I was working on:
__getattr__and friends.I could go on and on. It seems like the bar for merging a useful warning was just unfathomably low.
I’ve found myself agreeing with this sentiment more and more as I’ve gotten older. Especially this part:
That was burned into my thought process by this incident.
That depends a bit on the kind of fix. If it’s simply adding no lint comments then it can’t introduce a bug, only hide one from static analysers. For example, in CHERIoT we run clang-tidy to check a bunch of things and the most common false positive is using macro arguments without wrapping them in brackets. The warning isn’t clever enough to understand that some arguments are type names and so putting them in brackets would break and we add no-lint comments on these. This is also a hint to a future code reviewer that someone has thought about this use.
The warning from the example is weird though. In C, a T* might be a pointer to a T or to an array of T, so a lot of uses of a T* should expect to handle more than sizeof(T) data. I can’t imagine it not giving a load of false positives. That said, a coding style that doesn’t let a static analyser (or reader of the code) understand the bounds of a pointee makes me wince somewhat.
Deciding whether a warning should be suppressed can also take quite a bit of time to investigate, assuming a good-faith effort is done. This can be very fatiguing when you have to do this over and over, and also to keep suppressing the same warning every single time is just a hassle too. Disabling it project-wide might not be a good idea since not every instance will be a false positive.
Yup, it’s a trade and it’s the same one that the article discusses, but silencing a warning with a comment will not add bugs to a project.
Honestly this just reads as an argument for why static analysis in C is doomed to failure due to poor language design choices. In modern languages such as TypeScript, Rust, etc., I tend to pick the strictest static analysis settings for greenfield projects and have found 0 downsides - linting legacy code is a slightly different matter and some compromises must be taken in that case, but it just seems like for C you might as well not bother.
The author cropped out most of the code in that screenshot so it’s not really possible to understand the warning, but it seems uselessly broad. The nature of C is that you’re often casting pointers.
I’ve never used CodeQL, but the Clang static analyzer is pretty good; in medium-size C++ codebases I work on it might find one bogus issue per 10K LOC. often those can be silenced by adding an assertion, when the analyzer finds an issue on a code path that can’t actually occur.
The nature of C (and C++) is that warnings are essential; there is so much clearly-wrong code that’s allowed by the language spec, mostly for historical reasons. The first thing I do in any new project is crank up warnings to, yes, 11 (
-Wpedantic -Werror).The trend of tools over skills/experience certainly speaks to that. I think we’re at the peak of the “tools will save the industry!” belief. They help quite a bit but they can’t make a junior dev into a senior one.
Will be interesting to see if the “tools > everything else” belief holds up once AI-written code has proliferated. Will we grant AI slack because it’s not human? Or require AIs to only write code that conforms to all our tools? Who will fix the AI generated code so it passes all the tooling checks?
I strongly agree with this. In addition:
if you have warnings that you don’t deal with, there is a much greater chance that real issues will drown in the sea of noise.
but having a zero tolerance policy on warnings can be equally problematic. As well as the time it takes, as mentioned in the post, there are other noise effects it introduces. For example, silencing linter often involves adding comments to code, making your code noisier. It also means you are likely to choose colour themes in comments are made low contrast, which is the opposite of what you want (despite being done everywhere), because it leads to the genuinely important stuff being ignored.
One of the things I found amazing working with hardware folks was that they just expect Verilog to spit out tens of thousands of warnings. I learned this when we had a problem that the reset line on the network interface was wired high and so the network didn’t work. It turned out that the Verilog compiler had been warning that it was in an undefined state for ages, but it connects undefined things to whatever happens to be most convenient and that was low until another change made it high. The previous bug was that it was impossible to reset the NIC, but we never did that without reprogramming the FPGA anyway. The new one was that the NIC never came out of reset. The compiler clearly warned about the problem, but it also warned about thousands of things that were not important and so no one even bothered reading the warning.