Hacker News Reader

Comment by dataflow | Hacker News Reader

> 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

View on HN

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.

> 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.

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.

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.

How can the build system differentiate between new and old errors? Maybe I misunderstand you, but it seems like that piece is missing.

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.

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.

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.

You should test new compiler versions before updating your CI setup. If you get new warnings, you fix them before deploying.

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.

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.

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.

> 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.

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.

Please let me know so I can fix issues. Or let the compiler vendor know, sometimes they add something new to get feedback on how useful it is in the real world.

That is what I do. Right after I turn of -Werror so I can get my work done first…

You can still use -Werror in your CI on compilers you do support. Just don't enable that flag for everyone else.

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 ...

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.

> 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.

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.

Most companies have such teams, fortunate of those that never experienced such employers.

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.

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.

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.

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.

> 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.

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)

Good to know that my knowledge was dated.

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

A couple are kind of annoying, such as sign conversion/comparison.

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.

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.

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.

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.

The issue is that not every snapshot of a source file is intended to be committed as-is.

So it really depends if you're testing code or committing it. When you're debugging something locally, warnings like "unused parameter" or "dead code" become extremely annoying as errors. I need to be able to put "return 0;" in the middle of my function and run it as-is without having to battle the toolchain for five minutes after that every damn time.

Of course i don't have the specifics of your usual configuration. But usually this mostly a configuration problem. Dev builds usually don't have -Werror turned on ( or have a very easy way to turn Werror off). In most project i worked on, Werror is informance on the release, and continious test builds.

But i do agree that Werror on the code/build/test cycle sound like a pain.

I mean sure, you can work around any problem if you put enough effort into it. For this particular problem, in reality it introduces friction. e.g., one practical problem you face (if you've put sufficient work into your tooling to get to this point) is that your "dev" builds will now miss the cache far more often, because CI is running with different flags. Again, you can work around that... by (for example) spending more money and energy building with different flags. Nothing is impossible to do with enough investment, but these don't come free.

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.

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.

C/C++ build systems tend to be super noisy printing every file that they compile

  make --quiet

VERBOSE=0 is even the default with cmake

CMake prints every file.

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...

This is what an IDE can help with ;)

yeah sure, then I'll get a bunch of errors about stray j's and k's :)

Reply on news.ycombinator.com