7 ms·

(Don't) crank up the warnings to 11

82 points by jjgreen 4y ago

deleted 4y ago
[deleted]
remexre 4y ago
Iunno, for this example, isn't this actually UB according to standard C? I don't think it'd be that objectionable to put e.g. a semantic comment saying "yeah no this is actually not UB because we check in ./configure that your compiler is one known to not do bad things," *if* such a thing were standardized across linting tools. Of course, that's a heckuva if.
- david2ndaccount 4y ago
  This is not standard C, this is already in the land of compiler extensions and platform specific code. The behavior is defined by the compiler and no compiler should treat this as UB.
  - remexre 4y ago
    Is the code actually compiled with -fno-strict-aliasing (which IME is rare for userspace code, but I don't work with numerical code very much), or are you saying this is because of the types involved in the cast? I didn't see anything in [0] about a change in pointer semantics from using the vector extensions in the compilation unit? [0]: https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html#Vector-Extensions https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html#Ve...
    - zokier 4y ago
      _m512i is defined as may_alias https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i386/avx512cdintrin.h#L43 https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i38... https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#Common-Type-Attributes https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.ht...
  - deathanatos 4y ago
    It is using compiler intrinsics, I grant, and taking advantage of IB is yes, well-defined given an implementation (as we have here) … but it seems like it's still C? Here, I'm not 100% certain this code isn't UB: we type-pun a __m512i and a uint64_t … I'm not certain that's not UB. If I'm forced into writing C, I try to avoid doing that at all costs … I'd rather not encounter nasal demons. The warning also seems a bit merited, given that each "increment" is over more than a single object on the underlying array. (But, of course, that is the intent. But I'm just not sure the static analyzer can see it.)
    - cpeterso 4y ago
      Plus, a pointer to __m512i might require different alignment than a pointer to unsigned long. This code received a pointer to unsigned long from somewhere that might have cared about the data it pointed to, so his code should acknowledge that (with type casts or assertions) or make a __m512i or void* pointer part of this function's signature, making the type change part of the contract with the caller.
    - aidenn0 4y ago
      Someone can perhaps update me for C11, which I'm not up-to-date on, but at least as of C99, while the details of the specification was perhaps unclear on the specifics, the intent was clear that type-punning (of non-character types) without using unions is UB.
    - brigade 4y ago
      It’s not potentially UB until you access the object through the wrong pointer type; pointer math isn’t type punning. In this case it should be accessed through one of the load intrinsics, which has its behavior defined in the same way memcpy doesn’t invoke UB.
    - zokier 4y ago
      > Here, I'm not 100% certain this code isn't UB: we type-pun a __m512i and a uint64_t … I'm not certain that's not UB I'm 99% certain its not UB in itself, because of https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i386/avx512cdintrin.h#L41-L43 https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i38...
- eklitzke 4y ago
  This is not UB. In this case where you're using the wrong pointer type the worst thing that can happen is you can have aliasing issues. For example, if I have an int* and a double*, the C compiler is free to assume that the pointers can never alias each other so it can optimize loads and stores assuming that an update to the int pointer will never affect the double pointer. In general this can cause problems if the pointers actually do alias each other, but if there's no aliasing occurring then nothing bad will happen.
  - remexre 4y ago
    My understanding was that the UB was defined such that: int i = 0; float f = 1.0f; void* p; if(rand() & 1) p = &i; else p = &f; int j = *(int*)p; assert(i == j); could legally be compiled to a no-op (or a single call to rand() with an ignored result, I suppose), despite there being no "weird" pointer stores, since the ill-typed-at-runtime load "can't happen," so the compiler can reason that (int*)p must equal &i, and hence j = i. EDIT: And, in fact, gcc does this: https://godbolt.org/z/WM33vzzdo https://godbolt.org/z/WM33vzzdo Not sure if that applies to vector loads, and the gcc docs don't specify, though.
2h 4y ago
Or, maybe stop doing pointer arithmetic? We have had some pretty good new languages appear in the last 7 years. I know people are averse to change, me included. But you may want to ask yourself if continuing to use a 50 year old language is the right answer.
- eklitzke 4y ago
  This example isn't doing pointer arithmetic.
  - jeffbee 4y ago
    Yes, it is. const __m512i *ptr1 = (const __m512i*)container1->words; Then later: __m512i r1 = _mm512_loadu_si512(ptr1+i);
- zaptheimpaler 4y ago
  This is low level code written in a library to make full use of AVX-512.. which is inherently a processor native feature that has to be written in a language close to assembly because the processor only understands machine code.. there is no way to avoid assembly or the pointer arithmetic. Someone has to do the pointer arithmetic so the higher level languages and libraries don't have to.
- loup-vaillant 4y ago
  Well, then the proper response from Github would be something like: WARNING: HIGH-TO-CRITICAL SEVERITY: You are using C. C is not memory safe, and likely to expose your users to critical vulnerabilities. Not sure it would fly.
- Snetry 4y ago
  Oh how correct you are. Let us throw away all the existing code written in languages made in a different age and use languages that are completely binary incompatible just because we can.
ChrisMarshallNY 4y ago
> Training young programmers to avoid non-issues may make them less productive. I’ve found that training good habits, pays off in spades. Once a technique becomes habit, its overhead basically drops to 0. May take bit to get there, but once it’s ingrained, it is cost-free. An example is when I trained myself to switch from Whitesmiths indenting, to K&R indenting (when I switched from C++, to Swift). It took a few weeks of discomfort, but it is totally ingrained, these days.
- swader999 4y ago
  Why not just make your tool apply the indenting style? Then it's automatically baked into the team and project, no need for debates or merge issues.
  - ChrisMarshallNY 4y ago
    Sure, but I needed to do it, anyway. K&R has basically won the fight, so it's a no-brainer. Like I said, once I got in the habit, the overhead dropped to 0. Same for my code commenting and structuring, as well as many of the "little things" that I do, every day.
  - wyldfire 4y ago
    If your editor can do it, that's great. But another option is to apply it on commit [1]. [1] https://github.com/barisione/clang-format-hooks https://github.com/barisione/clang-format-hooks
adastra22 4y ago
I have turned off all GitHub scanner notifications. There is just too much noise and my time is limited.
bragr 4y ago
I don't know anything about the code quality of the author, but in my experience, the ones that complain about static checking and other guard rails the most, are the ones that need it the most, because in their hubris of thinking they won't make those kinds of errors, they make the most egregious forms of those errors. e.g. smart enough take pot shots about the possible downsides of the process, but not smart enough (or self aware enough) to see their own fallibility.
- jjgreen 4y ago
  There's a link to Professor Lemire's bio on the linked article: https://lemire.me/en/ https://lemire.me/en/
  - deleted 4y ago
    [deleted]
  - deleted 4y ago
    [deleted]
  - mhh__ 4y ago
    He's good to very good when talking about performance and simd tricks but I don't know if he's particularly sage when it comes to general programming practice. In this case I think he's probably wrong about compiler warnings but basically right about a lot of static analysers.
    - nkurz 4y ago
      I'm not sure of your familiarity, but I've worked with Daniel on several projects, and think you might be guessing wrong about his attitude toward warnings. He's not the sort who ignores warnings. His tendency is to change his code so they don't happen, even if he doesn't think a particular warning is helpful or necessary. I'm pretty sure his point isn't that the end user should get in the habit of ignoring warnings, but that those who decide which warnings are produced at which level need to be conscious that adding too many false positives can be counterproductive. His target audience here isn't application programmers, but those who decide which warnings are made default.
omoikane 4y ago
> Modifying code to fix a non-issue has a non-zero chance of introducing a real bug One example of such bug, where fixing a purify warning about uninitialized memory crippled OpenSSL: https://www.schneier.com/blog/archives/2008/05/random_number_b.html https://www.schneier.com/blog/archives/2008/05/random_number... via: https://xkcd.com/424/ https://xkcd.com/424/ I think it's generally preferred to enable most warnings but have some annotations to silence false positives, preferably with sufficient comments accompanying those annotations to help the code reviewers.
- plorkyeran 4y ago
  I've never thought that's a very good example of introducing a bug while trying to fix warnings. The warning was correctly pointing out that the code was broken, and the obviously incorrect "fix" merely made it never work instead of sometimes work, but sometimes working really wasn't good enough either.
zX41ZdbW 4y ago
The order should be as follows: - automated testing; - -Wall -Wextra, and -Weverything with some exceptions; - address, memory, thread, and undefined sanitizers; - fuzzing (at least some sort of), coverage-guided fuzzing with sanitizers; - static analysis, starting with clang-tidy; - other static analyzers, such as Coverity, CodeQL - have the least priority. An example of how everything of the above, everywhere, all at once is applied in public can be seen in the ClickHouse repository: https://github.com/ClickHouse/ClickHouse https://github.com/ClickHouse/ClickHouse in the per-commit checks. But keep in mind that the checks are really heavy - run for multiple hours, and we have to apply kludges to support GitHub Actions with spot instances... Also the dashboard can be checked here: https://aretestsgreenyet.com/ https://aretestsgreenyet.com/
- wahern 4y ago
  C and C++ developers who have been around long enough understand why clang specifically says not to regularly use -Weverything. Historically even -Wextra has resulted in spurious warnings for perfectly valid and clean code, as most if not all -Wextra warnings are heuristics. -Weverything is also all heuristics, but even more volatile and debatable. Any worthwhile -Weverything heuristic will eventually move to -Wextra. So, sure, use -Weverything on occasion to see if there's anything interesting to take note of. But if you include it by default you're inviting fatigue and unnecessary rewrites, which may be just as likely to result in the introduction of bugs as to fix any bugs.
  - gavinhoward 4y ago
    I think this depends on the person. I love -Weverything. I turn off two warnings by pragma in headers (-Wpadded, -Wc++98-compat), but other than that, I make the code obey all of warnings. This means things like explicitly marking unused arguments, marking pointer casts with a void pointer cast (with an assert to ensure alignment is okay), etc. But my code will build cleanly everywhere. That's really nice.
    - zamnos 4y ago
      Interesting. I'm from a different part of industry so x86_64 Linux is support is enough, where is "everywhere" for you?
jgrahamc 4y ago
I prefer maximum warnings and use of #pragma to disable a warning at a specific point when I know the warning is useless/wrong. That way I can get to a totally clean build with zero warnings output. The last significant piece of C I wrote: # Turn on absolutely all warnings and turn them into errors CFLAGS += -g -Wall -Wextra -Wno-unused-parameter -Werror https://github.com/cloudflare/keyless/blob/master/Makefile#L44 https://github.com/cloudflare/keyless/blob/master/Makefile#L... The reason I do this is if I leave some warnings in I can get complacent "oh, there's always a few warnings" or "that's expected" and then miss something. So, I like all the warnings and warnings treated as errors.
- Snetry 4y ago
  well, clearly not "maximum warnings" since you are missing a lot of them. -pedantic -Wunreachable-code -Wcast-qual -Wswitch-default -Wconversion -Wshadow -Wstrict-aliasing -Winit-self -Wcast-align -Wpointer-arith -Wmissing-declarations -Wmissing-include-dirs -Wno-unused-parameter -Wuninitialized and thats not even all that are useful. Also, don't use -Werror if you can Warnings will change based on compiler, compiler version and a countless number of other factors that make working with code that turns them into errors a pain.
  - throwway120385 4y ago
    I actually do what parent does for work projects but I also control the compiler and can decide when to upgrade. Like everything it's a trade-off and if I wanted wide support for different platforms I would consider your advice.
    - Snetry 4y ago
      Do you only control the compiler or the entire toolchain and system? If only the compiler you are still on very shaky grounds with -Werror. Even for the Linux kernel devs -Werror is too harsh.
  - flohofwoe 4y ago
    A good strategy is to use -Werror in development mode and CI builds, but not in 'distribution mode' (or alternatively: use -Werror for your own code, but not for imported libraries).
ok123456 4y ago
Compiler generated warnings are generally pretty high quality. Dynamic analysis tools generate high quality warnings. Static analysis tools that work on a significant amount of the AST seem to generally give good enough warnings. It's the shallow pattern based, heuristic tools seem to generate a lot of noise.
teddyh 4y ago
> However, I fear that it is part of a larger trend where people come to rely more or more on overbearing static analysis to judge code quality. The more warnings, the better, they think. Don’t get me started on security scanners which basically report all “open” ports as a security issue.
jeffbee 4y ago
The PR in question seems to be https://github.com/RoaringBitmap/CRoaring/pull/446 https://github.com/RoaringBitmap/CRoaring/pull/446 I agree that the GitHub analyses are worse than useless, but I also think it highlights the friction between intrinsics and the program in which they appear. There probably is a better model.
- nkurz 4y ago
  I think this "friction" is really important to understand. For code of this sort, usually you figure out what assembly you want produced, and then you figure out the intrinsics necessary to produce this. In your mind, there is a clear one-to-one correspondence between the what you write and the assembly you want generated, but too frequently the compiler may have other ideas. It constantly feels like your are fighting the compiler's attempts to subvert you. Personally, I think this best current solution is writing blocks or entire functions in assembly, but admittedly the tools for this aren't great.
aidenn0 4y ago
Right thinking people can also disagree on what is a false-positive. I remember a dialog between two people about one such thing. The summary is: 1. No improper behavior occurs currently because of the item flagged 2. However, reasonable changes to other parts of the code could cause the problem to occur 3. There is an obvious way of rewriting that avoids #2. One person argued it was a false positive because of #1, while the other argued it was a true positive because #2 and #3 combined means you definitely want to change the code...
- Gigachad 4y ago
  Our general rule is that if you think the linter/warnings/style preference is wrong, open a PR up with the upstream linter project and convince them they should remove it. Otherwise just change the submission to pass.
Ameo 4y ago
So much dev time goes up in smoke because of pedantic and overzealous linters, formatters, etc. in PR checks and CI pipelines. Your whole PR gets held up because of a style nit like an unused import or trailing whitespace. That forces you to pull up the PR in Github, dig into the CI failure, check out your branch again, make the fix, commit, push, and wait for CI all over again. If your PR was previously approved, then you have to bug the reviewer to re-approve since the review gets dismissed when you push new commits. I know this isn't exactly what this article was talking about, but it's such a big frustration to me. I'm all for leaving warnings like this on and cleaning them up over time, but yeah I strongly believe that turning all lints and checks and warnings all the way up and then gating PRs behind them is a recipe for huge developer frustration and loss of velocity.
- mhh__ 4y ago
  A lot of this is a symptom of CIs being deliberately dumb to avoid being wrong, and our workflows being shit. Fuck the CI, the velocity is quite bad just from the programmers machine. One small thing that arcanist (random example) does that raw git does not is making it very "automatic" to make a really simple change to some code. Even theoretically very trivial things like guessing which files you meant to add to the commit can be a real time saver. In fact, contrary to what I just wrote, it's not even entirely about time - a lot of this is really about mental context switching. (Also it's not universally true that you need re-approval)
- jacobsenscott 4y ago
  We have a fairly good middle ground - our linters only alert on changed lines. They run locally with a commit hook, so you can fix them before they hit the build. A clean linter run isn't required to merge, we trust devs to review the warnings and decide if they should fix them. Also you can turn off the github "feature" that dismisses the review when you push a new change. Trust devs to only make trivial changes after the review - or to re-request the review themselves if they make bigger changes.
- Centigonal 4y ago
  Sounds like your team may benefit from pre-commit hooks! Our team has an onboarding script we have every new dev run that (among other things) installs pre-commit. Our main repo includes hooks for style, max file size, etc. If there's a style nit, git will yell at you and not accept the commit. If possible, it'll actually correct the style issue (e.g. blackify a python file for you) and you can just stage the updated file. We still have CI/CD, but that catches other errors, not linting issues. This method has worked pretty well for us so far.
https443 4y ago
This is why the NPM warnings are such a problem too. https://overreacted.io/npm-audit-broken-by-design/ https://overreacted.io/npm-audit-broken-by-design/
gumby 4y ago
When I have "start from an empty buffer" code I try to crank the warnings up to "ten" at least. It helps a lot. But I agree that all the way to "eleven" is usually counterproductive. And rarely does one get to start with an empty buffer, especially given you're going to #include something.
WirelessGigabit 4y ago
They're CodeQL warnings. On GitHub. They don't happen during development. Nothing is telling you to have to have Warnings dialed up to 11 when coding. But at least have the courtesy to run them on your draft PR (if manual runs are needed) or make them part of the build process. Warnings are there because many people got bit in the rear by them. Warnings tell you places where you're not explicit. Explicitness is good. But when and where you go through the warnings is up to you. As long as you do it before other people review the code.

82 points by jjgreen 4y ago

deleted 4y ago

[deleted]

Iunno, for this example, isn't this actually UB according to standard C? I don't think it'd be that objectionable to put e.g. a semantic comment saying "yeah no this is actually not UB because we check in ./configure that your compiler is one known to not do bad things," *if* such a thing were standardized across linting tools. Of course, that's a heckuva if.

david2ndaccount 4y ago
This is not standard C, this is already in the land of compiler extensions and platform specific code. The behavior is defined by the compiler and no compiler should treat this as UB.
- remexre 4y ago
  Is the code actually compiled with -fno-strict-aliasing (which IME is rare for userspace code, but I don't work with numerical code very much), or are you saying this is because of the types involved in the cast? I didn't see anything in [0] about a change in pointer semantics from using the vector extensions in the compilation unit? [0]: https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html#Vector-Extensions https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html#Ve...
  - zokier 4y ago
    _m512i is defined as may_alias https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i386/avx512cdintrin.h#L43 https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i38... https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#Common-Type-Attributes https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.ht...
- deathanatos 4y ago
  It is using compiler intrinsics, I grant, and taking advantage of IB is yes, well-defined given an implementation (as we have here) … but it seems like it's still C? Here, I'm not 100% certain this code isn't UB: we type-pun a __m512i and a uint64_t … I'm not certain that's not UB. If I'm forced into writing C, I try to avoid doing that at all costs … I'd rather not encounter nasal demons. The warning also seems a bit merited, given that each "increment" is over more than a single object on the underlying array. (But, of course, that is the intent. But I'm just not sure the static analyzer can see it.)
  - cpeterso 4y ago
    Plus, a pointer to __m512i might require different alignment than a pointer to unsigned long. This code received a pointer to unsigned long from somewhere that might have cared about the data it pointed to, so his code should acknowledge that (with type casts or assertions) or make a __m512i or void* pointer part of this function's signature, making the type change part of the contract with the caller.
  - aidenn0 4y ago
    Someone can perhaps update me for C11, which I'm not up-to-date on, but at least as of C99, while the details of the specification was perhaps unclear on the specifics, the intent was clear that type-punning (of non-character types) without using unions is UB.
  - brigade 4y ago
    It’s not potentially UB until you access the object through the wrong pointer type; pointer math isn’t type punning. In this case it should be accessed through one of the load intrinsics, which has its behavior defined in the same way memcpy doesn’t invoke UB.
  - zokier 4y ago
    > Here, I'm not 100% certain this code isn't UB: we type-pun a __m512i and a uint64_t … I'm not certain that's not UB I'm 99% certain its not UB in itself, because of https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i386/avx512cdintrin.h#L41-L43 https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i38...
eklitzke 4y ago
This is not UB. In this case where you're using the wrong pointer type the worst thing that can happen is you can have aliasing issues. For example, if I have an int* and a double*, the C compiler is free to assume that the pointers can never alias each other so it can optimize loads and stores assuming that an update to the int pointer will never affect the double pointer. In general this can cause problems if the pointers actually do alias each other, but if there's no aliasing occurring then nothing bad will happen.
- remexre 4y ago
  My understanding was that the UB was defined such that: int i = 0; float f = 1.0f; void* p; if(rand() & 1) p = &i; else p = &f; int j = *(int*)p; assert(i == j); could legally be compiled to a no-op (or a single call to rand() with an ignored result, I suppose), despite there being no "weird" pointer stores, since the ill-typed-at-runtime load "can't happen," so the compiler can reason that (int*)p must equal &i, and hence j = i. EDIT: And, in fact, gcc does this: https://godbolt.org/z/WM33vzzdo https://godbolt.org/z/WM33vzzdo Not sure if that applies to vector loads, and the gcc docs don't specify, though.

2h 4y ago

Or, maybe stop doing pointer arithmetic? We have had some pretty good new languages appear in the last 7 years. I know people are averse to change, me included. But you may want to ask yourself if continuing to use a 50 year old language is the right answer.

eklitzke 4y ago
This example isn't doing pointer arithmetic.
- jeffbee 4y ago
  Yes, it is. const __m512i *ptr1 = (const __m512i*)container1->words; Then later: __m512i r1 = _mm512_loadu_si512(ptr1+i);
zaptheimpaler 4y ago
This is low level code written in a library to make full use of AVX-512.. which is inherently a processor native feature that has to be written in a language close to assembly because the processor only understands machine code.. there is no way to avoid assembly or the pointer arithmetic. Someone has to do the pointer arithmetic so the higher level languages and libraries don't have to.
loup-vaillant 4y ago
Well, then the proper response from Github would be something like: WARNING: HIGH-TO-CRITICAL SEVERITY: You are using C. C is not memory safe, and likely to expose your users to critical vulnerabilities. Not sure it would fly.
Snetry 4y ago
Oh how correct you are. Let us throw away all the existing code written in languages made in a different age and use languages that are completely binary incompatible just because we can.

ChrisMarshallNY 4y ago

> Training young programmers to avoid non-issues may make them less productive. I’ve found that training good habits, pays off in spades. Once a technique becomes habit, its overhead basically drops to 0. May take bit to get there, but once it’s ingrained, it is cost-free. An example is when I trained myself to switch from Whitesmiths indenting, to K&R indenting (when I switched from C++, to Swift). It took a few weeks of discomfort, but it is totally ingrained, these days.

swader999 4y ago
Why not just make your tool apply the indenting style? Then it's automatically baked into the team and project, no need for debates or merge issues.
- ChrisMarshallNY 4y ago
  Sure, but I needed to do it, anyway. K&R has basically won the fight, so it's a no-brainer. Like I said, once I got in the habit, the overhead dropped to 0. Same for my code commenting and structuring, as well as many of the "little things" that I do, every day.
- wyldfire 4y ago
  If your editor can do it, that's great. But another option is to apply it on commit [1]. [1] https://github.com/barisione/clang-format-hooks https://github.com/barisione/clang-format-hooks

adastra22 4y ago

I have turned off all GitHub scanner notifications. There is just too much noise and my time is limited.

bragr 4y ago

I don't know anything about the code quality of the author, but in my experience, the ones that complain about static checking and other guard rails the most, are the ones that need it the most, because in their hubris of thinking they won't make those kinds of errors, they make the most egregious forms of those errors. e.g. smart enough take pot shots about the possible downsides of the process, but not smart enough (or self aware enough) to see their own fallibility.

jjgreen 4y ago
There's a link to Professor Lemire's bio on the linked article: https://lemire.me/en/ https://lemire.me/en/
- deleted 4y ago
  [deleted]
- deleted 4y ago
  [deleted]
- mhh__ 4y ago
  He's good to very good when talking about performance and simd tricks but I don't know if he's particularly sage when it comes to general programming practice. In this case I think he's probably wrong about compiler warnings but basically right about a lot of static analysers.
  - nkurz 4y ago
    I'm not sure of your familiarity, but I've worked with Daniel on several projects, and think you might be guessing wrong about his attitude toward warnings. He's not the sort who ignores warnings. His tendency is to change his code so they don't happen, even if he doesn't think a particular warning is helpful or necessary. I'm pretty sure his point isn't that the end user should get in the habit of ignoring warnings, but that those who decide which warnings are produced at which level need to be conscious that adding too many false positives can be counterproductive. His target audience here isn't application programmers, but those who decide which warnings are made default.

omoikane 4y ago

> Modifying code to fix a non-issue has a non-zero chance of introducing a real bug One example of such bug, where fixing a purify warning about uninitialized memory crippled OpenSSL: https://www.schneier.com/blog/archives/2008/05/random_number_b.html https://www.schneier.com/blog/archives/2008/05/random_number... via: https://xkcd.com/424/ https://xkcd.com/424/ I think it's generally preferred to enable most warnings but have some annotations to silence false positives, preferably with sufficient comments accompanying those annotations to help the code reviewers.

plorkyeran 4y ago
I've never thought that's a very good example of introducing a bug while trying to fix warnings. The warning was correctly pointing out that the code was broken, and the obviously incorrect "fix" merely made it never work instead of sometimes work, but sometimes working really wasn't good enough either.

zX41ZdbW 4y ago

The order should be as follows: - automated testing; - -Wall -Wextra, and -Weverything with some exceptions; - address, memory, thread, and undefined sanitizers; - fuzzing (at least some sort of), coverage-guided fuzzing with sanitizers; - static analysis, starting with clang-tidy; - other static analyzers, such as Coverity, CodeQL - have the least priority. An example of how everything of the above, everywhere, all at once is applied in public can be seen in the ClickHouse repository: https://github.com/ClickHouse/ClickHouse https://github.com/ClickHouse/ClickHouse in the per-commit checks. But keep in mind that the checks are really heavy - run for multiple hours, and we have to apply kludges to support GitHub Actions with spot instances... Also the dashboard can be checked here: https://aretestsgreenyet.com/ https://aretestsgreenyet.com/

wahern 4y ago
C and C++ developers who have been around long enough understand why clang specifically says not to regularly use -Weverything. Historically even -Wextra has resulted in spurious warnings for perfectly valid and clean code, as most if not all -Wextra warnings are heuristics. -Weverything is also all heuristics, but even more volatile and debatable. Any worthwhile -Weverything heuristic will eventually move to -Wextra. So, sure, use -Weverything on occasion to see if there's anything interesting to take note of. But if you include it by default you're inviting fatigue and unnecessary rewrites, which may be just as likely to result in the introduction of bugs as to fix any bugs.
- gavinhoward 4y ago
  I think this depends on the person. I love -Weverything. I turn off two warnings by pragma in headers (-Wpadded, -Wc++98-compat), but other than that, I make the code obey all of warnings. This means things like explicitly marking unused arguments, marking pointer casts with a void pointer cast (with an assert to ensure alignment is okay), etc. But my code will build cleanly everywhere. That's really nice.
  - zamnos 4y ago
    Interesting. I'm from a different part of industry so x86_64 Linux is support is enough, where is "everywhere" for you?

jgrahamc 4y ago

I prefer maximum warnings and use of #pragma to disable a warning at a specific point when I know the warning is useless/wrong. That way I can get to a totally clean build with zero warnings output. The last significant piece of C I wrote: # Turn on absolutely all warnings and turn them into errors CFLAGS += -g -Wall -Wextra -Wno-unused-parameter -Werror https://github.com/cloudflare/keyless/blob/master/Makefile#L44 https://github.com/cloudflare/keyless/blob/master/Makefile#L... The reason I do this is if I leave some warnings in I can get complacent "oh, there's always a few warnings" or "that's expected" and then miss something. So, I like all the warnings and warnings treated as errors.

Snetry 4y ago
well, clearly not "maximum warnings" since you are missing a lot of them. -pedantic -Wunreachable-code -Wcast-qual -Wswitch-default -Wconversion -Wshadow -Wstrict-aliasing -Winit-self -Wcast-align -Wpointer-arith -Wmissing-declarations -Wmissing-include-dirs -Wno-unused-parameter -Wuninitialized and thats not even all that are useful. Also, don't use -Werror if you can Warnings will change based on compiler, compiler version and a countless number of other factors that make working with code that turns them into errors a pain.
- throwway120385 4y ago
  I actually do what parent does for work projects but I also control the compiler and can decide when to upgrade. Like everything it's a trade-off and if I wanted wide support for different platforms I would consider your advice.
  - Snetry 4y ago
    Do you only control the compiler or the entire toolchain and system? If only the compiler you are still on very shaky grounds with -Werror. Even for the Linux kernel devs -Werror is too harsh.
- flohofwoe 4y ago
  A good strategy is to use -Werror in development mode and CI builds, but not in 'distribution mode' (or alternatively: use -Werror for your own code, but not for imported libraries).

ok123456 4y ago

Compiler generated warnings are generally pretty high quality. Dynamic analysis tools generate high quality warnings. Static analysis tools that work on a significant amount of the AST seem to generally give good enough warnings. It's the shallow pattern based, heuristic tools seem to generate a lot of noise.

teddyh 4y ago

> However, I fear that it is part of a larger trend where people come to rely more or more on overbearing static analysis to judge code quality. The more warnings, the better, they think. Don’t get me started on security scanners which basically report all “open” ports as a security issue.

jeffbee 4y ago

The PR in question seems to be https://github.com/RoaringBitmap/CRoaring/pull/446 https://github.com/RoaringBitmap/CRoaring/pull/446 I agree that the GitHub analyses are worse than useless, but I also think it highlights the friction between intrinsics and the program in which they appear. There probably is a better model.

nkurz 4y ago
I think this "friction" is really important to understand. For code of this sort, usually you figure out what assembly you want produced, and then you figure out the intrinsics necessary to produce this. In your mind, there is a clear one-to-one correspondence between the what you write and the assembly you want generated, but too frequently the compiler may have other ideas. It constantly feels like your are fighting the compiler's attempts to subvert you. Personally, I think this best current solution is writing blocks or entire functions in assembly, but admittedly the tools for this aren't great.

aidenn0 4y ago

Right thinking people can also disagree on what is a false-positive. I remember a dialog between two people about one such thing. The summary is: 1. No improper behavior occurs currently because of the item flagged 2. However, reasonable changes to other parts of the code could cause the problem to occur 3. There is an obvious way of rewriting that avoids #2. One person argued it was a false positive because of #1, while the other argued it was a true positive because #2 and #3 combined means you definitely want to change the code...

Gigachad 4y ago
Our general rule is that if you think the linter/warnings/style preference is wrong, open a PR up with the upstream linter project and convince them they should remove it. Otherwise just change the submission to pass.

Ameo 4y ago

So much dev time goes up in smoke because of pedantic and overzealous linters, formatters, etc. in PR checks and CI pipelines. Your whole PR gets held up because of a style nit like an unused import or trailing whitespace. That forces you to pull up the PR in Github, dig into the CI failure, check out your branch again, make the fix, commit, push, and wait for CI all over again. If your PR was previously approved, then you have to bug the reviewer to re-approve since the review gets dismissed when you push new commits. I know this isn't exactly what this article was talking about, but it's such a big frustration to me. I'm all for leaving warnings like this on and cleaning them up over time, but yeah I strongly believe that turning all lints and checks and warnings all the way up and then gating PRs behind them is a recipe for huge developer frustration and loss of velocity.

mhh__ 4y ago
A lot of this is a symptom of CIs being deliberately dumb to avoid being wrong, and our workflows being shit. Fuck the CI, the velocity is quite bad just from the programmers machine. One small thing that arcanist (random example) does that raw git does not is making it very "automatic" to make a really simple change to some code. Even theoretically very trivial things like guessing which files you meant to add to the commit can be a real time saver. In fact, contrary to what I just wrote, it's not even entirely about time - a lot of this is really about mental context switching. (Also it's not universally true that you need re-approval)
jacobsenscott 4y ago
We have a fairly good middle ground - our linters only alert on changed lines. They run locally with a commit hook, so you can fix them before they hit the build. A clean linter run isn't required to merge, we trust devs to review the warnings and decide if they should fix them. Also you can turn off the github "feature" that dismisses the review when you push a new change. Trust devs to only make trivial changes after the review - or to re-request the review themselves if they make bigger changes.
Centigonal 4y ago
Sounds like your team may benefit from pre-commit hooks! Our team has an onboarding script we have every new dev run that (among other things) installs pre-commit. Our main repo includes hooks for style, max file size, etc. If there's a style nit, git will yell at you and not accept the commit. If possible, it'll actually correct the style issue (e.g. blackify a python file for you) and you can just stage the updated file. We still have CI/CD, but that catches other errors, not linting issues. This method has worked pretty well for us so far.

https443 4y ago

This is why the NPM warnings are such a problem too. https://overreacted.io/npm-audit-broken-by-design/ https://overreacted.io/npm-audit-broken-by-design/

gumby 4y ago

When I have "start from an empty buffer" code I try to crank the warnings up to "ten" at least. It helps a lot. But I agree that all the way to "eleven" is usually counterproductive. And rarely does one get to start with an empty buffer, especially given you're going to #include something.

WirelessGigabit 4y ago

They're CodeQL warnings. On GitHub. They don't happen during development. Nothing is telling you to have to have Warnings dialed up to 11 when coding. But at least have the courtesy to run them on your draft PR (if manual runs are needed) or make them part of the build process. Warnings are there because many people got bit in the rear by them. Warnings tell you places where you're not explicit. Explicitness is good. But when and where you go through the warnings is up to you. As long as you do it before other people review the code.