7 ms·

> Developers should additionally use -Werror I've yet to be convinced that this makes sense for every warning. It really depends on the warning IMO. Edit to e

> Developers should additionally use -Werror I've yet to be convinced that this makes sense for every warning. It really depends on the warning IMO. Edit to elaborate: I was mostly referring to non-committed/non-release code not deserving -Werror [1]. However, even for release builds, the story kind of depends on whether your builds are hermetic or not. If your commit also includes a snapshot of all your dependencies including your compiler toolchain executables, then off the top of my head, I can't think of any cases where you want warnings without errors (though perhaps there might be some). But if you're ever going to compile the same commit with a different toolchain (like say your system toolchain that you updated), then I don't think you want -Werror, otherwise every time the warning catches a new case, you'll fail to build something that previously compiled fine. [1] https://news.ycombinator.com/item?id=38481255 https://news.ycombinator.com/item?id=38481255

olig15 3y ago
Why, what’s the alternative - leave the warnings hanging around? I work on a multi million loc.C++ codebase that used -Werror. If we turned it off, the codebase would be full of warnings in hours.
- kevin_thibedeau 3y ago
  Use -Werror for the build server and have a policy that you can't commit code that generates warnings. That lets you still build locally without having to immediately fix minor warnings that would be a distraction.
  - yellow_lead 3y ago
    How can the build system differentiate between new and old errors? Maybe I misunderstand you, but it seems like that piece is missing.
    - kevin_thibedeau 3y ago
      If the policy is followed no errors will be generated on the build server. You have to have a codebase that builds cleanly for this to be possible so you're forced to resolve warnings before they become a cancer.
    - plorkyeran 3y ago
      There are no old warnings because the commits introducing them would have been rejected. Obviously a prerequisite of this is that you get your codebase to the point where it has no warnings. It is sort of like asking how the build server should differentiate between new test failures and existing test failures.
      - Am4TIfIsER0ppos 3y ago
        You update the compiler and suddenly the same code now generates warnings. Are these old warnings or new warnings? I have seen that with printing an integer in a loop 0..99. Old compilers didn't understand the limit so warned that my buffer was not big enough. Middle aged compilers were silent because I assume they understood the range. New compilers started warning again that the range was -2147483648..99 dumb shits. Or are you one of those to never update a working system, which I completely agree with.
        plorkyeran 3y ago
        You should test new compiler versions before updating your CI setup. If you get new warnings, you fix them before deploying.
- r2vcap 3y ago
  If you're working in a controlled environment, such as proprietary software with a fixed number of toolchains or a few OSS projects like Chromium that use their own fixed toolchain, then it makes sense to turn on -Werror. However, most OSS projects deal with tons of different compilers, so it is not possible to enforce -Werror, as different compilers and versions handle warnings differently.
  - soulbadguy 3y ago
    it takes a bit of discipline, but i think it is entirely possible. Even when some OSS project support a lot of different build environment, the support is usually tiered with some of the target having daily or per commit builds. Those should be setup with -Werror + exceptions where it makes sense. Once a project get to stable state (as in for a given target, all the warning are either turned off, or handled). Upgrading or adding new target get easier.
    - saagarjha 3y ago
      Even projects with a “stable” set of targets have too much churn for this to be practical. Just because you support “latest macOS” doesn’t mean my build should fail if I’m on a beta build ahead.
      - soulbadguy 3y ago
        > Even projects with a “stable” set of targets have too much churn for this to be practical. Are you speaking here from experience ? Because mine has been quite different.
        saagarjha 3y ago
        Yes, I typically run toolchains that are slightly ahead or different than the one that is actually used in CI. I don't think it is reasonable to think that projects will be using it, though they often do appreciate me testing it early for them.
      - 3y ago
- ploxiln 3y ago
  The alternative is to fix warnings! You don't have to completely fail the build, to have the good engineering sense to fix warnings. Unless you never build locally? And never look at a build log? You really never improve code if Werror doesn't force you to? It's really obnoxious to also immediately and completely fail the build for someone who wants to use an updated compiler, or wants to compile for a different architecture, or wants to compare to an older compiler, or a different vendor's compiler, or test with and updated library with changed headers, or ...
  - not2b 3y ago
    Turning on -Werror means that once the warnings are eliminated, they stay eliminated, and a developer who adds code that produces a warning has their checkin rejected. If it isn't used the number of warnings will just grow and grow. You're right, updated compilers that have more warnings are an issue, and that's why the document recommends that -Werror be used during development but not in the shipped code (for open source projects), so the recipient of the code isn't blocked by the problem you cite.
    - account42 3y ago
      > If it isn't used the number of warnings will just grow and grow. Only in shitty teams without discipline and only if warnings are not tracked in some other way.
      - not2b 3y ago
        The way to impose discipline effectively is to have automated checks. Like -Werror, and additional "lint"-style checks to make sure that coding guidelnes are followed. Non-shitty teams automate as much of the flow as possible, to catch mistakes early and help the human reviewers catch everything.
      - pjmlp 3y ago
        Most companies have such teams, fortunate of those that never experienced such employers.
IshKebab 3y ago
It definitely does for C/C++. I think there are two reasons: 1. C/C++ build systems tend to be super noisy printing every file that they compile, often long commands and usually in an ugly style, so it is very easy to just miss warnings or to give up even trying to look for them because the output is so verbose. 2. C/C++ tend to produce a lot of warnings that are very annoying to resolve properly and usually not worth the effort. Sign and size conversion warnings are probably the worst. They're low information warnings that usually don't have a clear solution, so people learn to just accept that some warnings should be ignored and they stop trying to fix warnings in general. 3. It's usually difficult to prevent warnings from third party code from being shown and you can't do much about those. There is `-isystem` but it's not well supported and it doesn't solve everything. I think Go got rid of warnings entirely because of how bad the warning experience in C/C++. But my experience of Rust has shown that warnings can be done sensibly so I wouldn't use `-Werror` in Rust. In C++ though, you should use `-Werror` in CI and then whitelist specific warnings. Just don't enable it for downstream users.
- kevin_thibedeau 3y ago
  I wouldn't be so careless about sign conversion warnings. C can silently convert signed integers to unsigned and that can cause mayhem. I inherited a codebase that was exhibiting a classic signed conversion problem when a temperature reading dropped below 0°C. I simply turned on -Wextra and it flagged all the culprits with implicit conversion to unsigned. Throw in some casts and it was resolved.
- jjgreen 3y ago
  C/C++ build systems tend to be super noisy printing every file that they compile make --quiet
  - account42 3y ago
    VERBOSE=0 is even the default with cmake
    - IshKebab 3y ago
cozzyd 3y ago
What would be more useful would be some better UI for displaying the warnings. e.g. -Woutput_warning_[html,json,text]=DIRECTORY_TO_OUTPUT_TO or something, which would create a output file per compilation unit that displayed the warnings in a friendlier way. It's possible something like this exists and I'm unaware...
- saagarjha 3y ago
  This is what an IDE can help with ;)
  - cozzyd 3y ago
    yeah sure, then I'll get a bunch of errors about stray j's and k's :)
david2ndaccount 3y ago
Indeed, -Werror doesn’t make sense, but specifying specific errors (like -Werror=return-type to prevent forgetting to return a value from a function) is very, very necessary.
- saagarjha 3y ago
  Yep, this is the best way to use it. Pick a few classes of things you absolutely want to always be errors, and add to it over time.
soulbadguy 3y ago
genuilely curious : Do you have a class of warnings or a warning in particular you think should be left out of Werror by default ?
- saagarjha 3y ago
  A couple are kind of annoying, such as sign conversion/comparison.
  - soulbadguy 3y ago
    Most security/safety feature either on the language or the tooling side are are some what annoying since by definition they tend to add constrains to one workflow. The real question is whether they introduce more pain than they safe in average.
    - saagarjha 3y ago
      Right, I understand that. I'm overwhelmingly in support for most warnings. I just think that a handful don't have good tradeoffs or are not necessarily the most valuable for many projects.
- humanrebar 3y ago
  The deprecated attribute is useless when -Werror and -Wall are both on. You can't land the attribute if any uses exist because any use is now a compiler error. You might as well just delete the entity and sort things out from there.
  - soulbadguy 3y ago
    +1
  - bluGill 3y ago
    Normally you can pragma turn the error off for each use. I'm meaning to dp this for stuff where our ABI must support the function but I don't want anyone to use it otherwise.
viraptor 3y ago
In the proposed context "developers should", yeah, it totally makes sense. You've got different categories of warnings: Things that are harmless but trivial to fix - so why not fix them. Things that are valid in this specific context - they probably deserve a comment / pragma / explicit disabling. Things that are real issues that should be fixed but won't stop the compilation - and if they're invisible because of the noise of the previous 3 categories, you're going to have issues. Just today had to fix two things which had warnings available for a long time and with the recent clang they became an error. They shouldn't been fixed upstream ages ago, but they were ok with the warnings.
- account42 3y ago
  > Things that are harmless but trivial to fix - so why not fix them. Because you're only trying out some experimental changes and are not ready to commit yet anyway. Making sure all corners are smoothened enough so that noone can cut himself makes no sense in that case.
bluGill 3y ago
It makes sense on my project where we have 20 million lines of C++, all covered by it. Only rarely does it stop my build on a I don't care (generally i'm adding temp debugging code that i'll rip out before commiting). If you keep clean code clean it isn't hard to leave werror on and even compiler upgrades are not that painful.
- Someone 3y ago
  > and even compiler upgrades are not that painful. …if you do them frequently enough. A problem with OSS code is that it may lie dormant for years, then get picked up by somebody. Even if they try to compile for the same CPU architecture with the same but newer compiler, getting things to compile with -Werror again can be a challenge. Changing architecture (e.g. integers becoming 64 bit, triggering lots of ‘possible truncation’ errors) or compiler makes it harder. I don’t think there’s a solution for that problem. Bit rot exists, and the longer you ignore it, the more work it is to get rid of it, and not trying to get rid of it is an invitation for having a serious warning getting drowned in harmless ones.
  - bluGill 3y ago
    I maintain 20 million lines of C++ and I don't update my compiler very often. There is some pain on update, but it is small and generally easy to fix. Part of this is I already build with gcc and clang (including clang static analisys), and also run cppcheck. Much of the complaints about upgrading compilers seem to be legacy from gcc 2.x days when they were adding a lot more warnings. Now gcc and clang both do a lot of testing against real world code to see if the warnings are noisy or not before adding them. (either that they are about visual studio or some other compiler I don't use)
    - Someone 3y ago
      Good to know that my knowledge was dated.
krylon 3y ago
It's been a long time since I've done anything in C, but I tended to use -Werror -Wall -Wextra and then disable individual warnings/errors if I was sure they were false positives. It can be tedious, but I figured better safe than sorry. EDIT: For the last couple of years I've used Go almost exclusively, where every warning is an error. It was annoying and tedious at first, but I've come to appreciate it.

olig15 3y ago

Why, what’s the alternative - leave the warnings hanging around? I work on a multi million loc.C++ codebase that used -Werror. If we turned it off, the codebase would be full of warnings in hours.

kevin_thibedeau 3y ago
Use -Werror for the build server and have a policy that you can't commit code that generates warnings. That lets you still build locally without having to immediately fix minor warnings that would be a distraction.
- yellow_lead 3y ago
  How can the build system differentiate between new and old errors? Maybe I misunderstand you, but it seems like that piece is missing.
  - kevin_thibedeau 3y ago
    If the policy is followed no errors will be generated on the build server. You have to have a codebase that builds cleanly for this to be possible so you're forced to resolve warnings before they become a cancer.
  - plorkyeran 3y ago
    There are no old warnings because the commits introducing them would have been rejected. Obviously a prerequisite of this is that you get your codebase to the point where it has no warnings. It is sort of like asking how the build server should differentiate between new test failures and existing test failures.
    - Am4TIfIsER0ppos 3y ago
      You update the compiler and suddenly the same code now generates warnings. Are these old warnings or new warnings? I have seen that with printing an integer in a loop 0..99. Old compilers didn't understand the limit so warned that my buffer was not big enough. Middle aged compilers were silent because I assume they understood the range. New compilers started warning again that the range was -2147483648..99 dumb shits. Or are you one of those to never update a working system, which I completely agree with.
      - plorkyeran 3y ago
        You should test new compiler versions before updating your CI setup. If you get new warnings, you fix them before deploying.
r2vcap 3y ago
If you're working in a controlled environment, such as proprietary software with a fixed number of toolchains or a few OSS projects like Chromium that use their own fixed toolchain, then it makes sense to turn on -Werror. However, most OSS projects deal with tons of different compilers, so it is not possible to enforce -Werror, as different compilers and versions handle warnings differently.
- soulbadguy 3y ago
  it takes a bit of discipline, but i think it is entirely possible. Even when some OSS project support a lot of different build environment, the support is usually tiered with some of the target having daily or per commit builds. Those should be setup with -Werror + exceptions where it makes sense. Once a project get to stable state (as in for a given target, all the warning are either turned off, or handled). Upgrading or adding new target get easier.
  - saagarjha 3y ago
    Even projects with a “stable” set of targets have too much churn for this to be practical. Just because you support “latest macOS” doesn’t mean my build should fail if I’m on a beta build ahead.
    - soulbadguy 3y ago
      > Even projects with a “stable” set of targets have too much churn for this to be practical. Are you speaking here from experience ? Because mine has been quite different.
      - saagarjha 3y ago
        Yes, I typically run toolchains that are slightly ahead or different than the one that is actually used in CI. I don't think it is reasonable to think that projects will be using it, though they often do appreciate me testing it early for them.
    - 3y ago
ploxiln 3y ago
The alternative is to fix warnings! You don't have to completely fail the build, to have the good engineering sense to fix warnings. Unless you never build locally? And never look at a build log? You really never improve code if Werror doesn't force you to? It's really obnoxious to also immediately and completely fail the build for someone who wants to use an updated compiler, or wants to compile for a different architecture, or wants to compare to an older compiler, or a different vendor's compiler, or test with and updated library with changed headers, or ...
- not2b 3y ago
  Turning on -Werror means that once the warnings are eliminated, they stay eliminated, and a developer who adds code that produces a warning has their checkin rejected. If it isn't used the number of warnings will just grow and grow. You're right, updated compilers that have more warnings are an issue, and that's why the document recommends that -Werror be used during development but not in the shipped code (for open source projects), so the recipient of the code isn't blocked by the problem you cite.
  - account42 3y ago
    > If it isn't used the number of warnings will just grow and grow. Only in shitty teams without discipline and only if warnings are not tracked in some other way.
    - not2b 3y ago
      The way to impose discipline effectively is to have automated checks. Like -Werror, and additional "lint"-style checks to make sure that coding guidelnes are followed. Non-shitty teams automate as much of the flow as possible, to catch mistakes early and help the human reviewers catch everything.
    - pjmlp 3y ago
      Most companies have such teams, fortunate of those that never experienced such employers.

IshKebab 3y ago

It definitely does for C/C++. I think there are two reasons: 1. C/C++ build systems tend to be super noisy printing every file that they compile, often long commands and usually in an ugly style, so it is very easy to just miss warnings or to give up even trying to look for them because the output is so verbose. 2. C/C++ tend to produce a lot of warnings that are very annoying to resolve properly and usually not worth the effort. Sign and size conversion warnings are probably the worst. They're low information warnings that usually don't have a clear solution, so people learn to just accept that some warnings should be ignored and they stop trying to fix warnings in general. 3. It's usually difficult to prevent warnings from third party code from being shown and you can't do much about those. There is `-isystem` but it's not well supported and it doesn't solve everything. I think Go got rid of warnings entirely because of how bad the warning experience in C/C++. But my experience of Rust has shown that warnings can be done sensibly so I wouldn't use `-Werror` in Rust. In C++ though, you should use `-Werror` in CI and then whitelist specific warnings. Just don't enable it for downstream users.

kevin_thibedeau 3y ago
I wouldn't be so careless about sign conversion warnings. C can silently convert signed integers to unsigned and that can cause mayhem. I inherited a codebase that was exhibiting a classic signed conversion problem when a temperature reading dropped below 0°C. I simply turned on -Wextra and it flagged all the culprits with implicit conversion to unsigned. Throw in some casts and it was resolved.
jjgreen 3y ago
C/C++ build systems tend to be super noisy printing every file that they compile make --quiet
- account42 3y ago
  VERBOSE=0 is even the default with cmake
  - IshKebab 3y ago

cozzyd 3y ago

What would be more useful would be some better UI for displaying the warnings. e.g. -Woutput_warning_[html,json,text]=DIRECTORY_TO_OUTPUT_TO or something, which would create a output file per compilation unit that displayed the warnings in a friendlier way. It's possible something like this exists and I'm unaware...

saagarjha 3y ago
This is what an IDE can help with ;)
- cozzyd 3y ago
  yeah sure, then I'll get a bunch of errors about stray j's and k's :)

david2ndaccount 3y ago

Indeed, -Werror doesn’t make sense, but specifying specific errors (like -Werror=return-type to prevent forgetting to return a value from a function) is very, very necessary.

saagarjha 3y ago
Yep, this is the best way to use it. Pick a few classes of things you absolutely want to always be errors, and add to it over time.

soulbadguy 3y ago

genuilely curious : Do you have a class of warnings or a warning in particular you think should be left out of Werror by default ?

saagarjha 3y ago
A couple are kind of annoying, such as sign conversion/comparison.
- soulbadguy 3y ago
  Most security/safety feature either on the language or the tooling side are are some what annoying since by definition they tend to add constrains to one workflow. The real question is whether they introduce more pain than they safe in average.
  - saagarjha 3y ago
    Right, I understand that. I'm overwhelmingly in support for most warnings. I just think that a handful don't have good tradeoffs or are not necessarily the most valuable for many projects.
humanrebar 3y ago
The deprecated attribute is useless when -Werror and -Wall are both on. You can't land the attribute if any uses exist because any use is now a compiler error. You might as well just delete the entity and sort things out from there.
- soulbadguy 3y ago
  +1
- bluGill 3y ago
  Normally you can pragma turn the error off for each use. I'm meaning to dp this for stuff where our ABI must support the function but I don't want anyone to use it otherwise.

viraptor 3y ago

In the proposed context "developers should", yeah, it totally makes sense. You've got different categories of warnings: Things that are harmless but trivial to fix - so why not fix them. Things that are valid in this specific context - they probably deserve a comment / pragma / explicit disabling. Things that are real issues that should be fixed but won't stop the compilation - and if they're invisible because of the noise of the previous 3 categories, you're going to have issues. Just today had to fix two things which had warnings available for a long time and with the recent clang they became an error. They shouldn't been fixed upstream ages ago, but they were ok with the warnings.

account42 3y ago
> Things that are harmless but trivial to fix - so why not fix them. Because you're only trying out some experimental changes and are not ready to commit yet anyway. Making sure all corners are smoothened enough so that noone can cut himself makes no sense in that case.

bluGill 3y ago

It makes sense on my project where we have 20 million lines of C++, all covered by it. Only rarely does it stop my build on a I don't care (generally i'm adding temp debugging code that i'll rip out before commiting). If you keep clean code clean it isn't hard to leave werror on and even compiler upgrades are not that painful.

Someone 3y ago
> and even compiler upgrades are not that painful. …if you do them frequently enough. A problem with OSS code is that it may lie dormant for years, then get picked up by somebody. Even if they try to compile for the same CPU architecture with the same but newer compiler, getting things to compile with -Werror again can be a challenge. Changing architecture (e.g. integers becoming 64 bit, triggering lots of ‘possible truncation’ errors) or compiler makes it harder. I don’t think there’s a solution for that problem. Bit rot exists, and the longer you ignore it, the more work it is to get rid of it, and not trying to get rid of it is an invitation for having a serious warning getting drowned in harmless ones.
- bluGill 3y ago
  I maintain 20 million lines of C++ and I don't update my compiler very often. There is some pain on update, but it is small and generally easy to fix. Part of this is I already build with gcc and clang (including clang static analisys), and also run cppcheck. Much of the complaints about upgrading compilers seem to be legacy from gcc 2.x days when they were adding a lot more warnings. Now gcc and clang both do a lot of testing against real world code to see if the warnings are noisy or not before adding them. (either that they are about visual studio or some other compiler I don't use)
  - Someone 3y ago
    Good to know that my knowledge was dated.

krylon 3y ago

It's been a long time since I've done anything in C, but I tended to use -Werror -Wall -Wextra and then disable individual warnings/errors if I was sure they were false positives. It can be tedious, but I figured better safe than sorry. EDIT: For the last couple of years I've used Go almost exclusively, where every warning is an error. It was annoying and tedious at first, but I've come to appreciate it.