Hacker News Reader

Comment by nkurz | Hacker News Reader

I tried to get involved in FFmpeg development several years ago, and felt driven off by the existing community. It wasn't so much any one individual, but the absence any "adult" voice trying to keep things moving forward. There are a lot of smart programmers there, and a lot of loud people on the list, but no real correspondence between the two traits.

The quality of the code base also left something to be desired. It was fast, and generally worked, but they certainly didn't subscribe to the camp that believes in treating compiler warnings as errors. I remember using Valgrind and discovering that some memory was being used uninitialized, and submitting a patch. The patch was rejected with a comment such as "not needed". I still don't know if this was correct, but I do know that being a static initializer my patch wasn't going to slow anything down.

View on HN

This really changed a lot, see http://fate.ffmpeg.org/

We now test all the code through valgrind; a single memory leak will complain. There are also static analysis, and various other checks.

And BTW, you can't treat warnings as errors. Just try to fix the warnings we have and you might understand why it's not possible easily (might require API changes, compiler false-positive, deprecated warnings that can just be silenced, etc). Of course, you might be able to fix some easily, and the patches will likely be accepted (assuming they are correct).

Glad to here that more testing is done. The "warnings as errors" is controversial, but it's practice I like. Once your compile has hundreds of "safe" warnings flashing across the screen, you aren't going to notice the "real" warning caused by your latest change. Whereby if your code compiles cleanly and quietly, you'll definitely notice the new one. Defaulting to -Werror is just a way to enforce this convention.

You are absolutely right that not all the warnings are accurate, and that not all code should be changed to satisfy a every compiler. But I'm arguing that there is a long-term benefit to choosing a single compiler, and making the modifications necessary to allow this compiler to run without warnings, even if the behaviour is provably correct without doing so.

I'm pretty sure a project as widely used as FFmpeg can't just choose a single compiler (and you'd actually have to choose a single compiler version since for instance new versions of GCC can introduce new warnings).

On top of that, sometimes warnings come from parts of the code you don't control, like a scanner generated by flex.

I think it's better to have a strong policy of not introducing warnings and fixing them whenever a particular compiler is foud to emit them, but having -Werror in the default Makefile can quickly inundate the project's mailing list/IRC channel with "doesn't compile" complaints from people with exotic compilers.

Mosh has an optional ./configure --enable-compile-warnings=error flag that turns on -Werror. It's off by default for users who compile from source, but the Debian, Ubuntu and Fedora packaging uses it to prepare the binary package. We think this is a good compromise.

We might be agreeing. I'm not arguing for -Werror on by default for a project that is designed to be compiled from source by end users using their own compiler. I am arguing that a clean compile is valuable for developers. Yes, this may get down to choosing versions.

I think SQLite has a good attitude: http://www.sqlite.org/faq.html#q17

"Some people say that we should eliminate all warnings because benign warnings mask real warnings that might arise in future changes. This is true enough. But in reply, the developers observe that all warnings have already been fixed in the compilers used for SQLite development (various versions of GCC). Compiler warnings only arise from compilers that the developers do not use on a daily basis (Ex: MSVC)."

Static initializers aren't always free, FYI. They can slow down app startup (this is a serious issue for Firefox).

I think the word static was misused in nkurz's post. static variables are always initialized, at least to 0. Initializing a stack variable shouldn't harm performance.

Correct, I was trying to describe initializing a stack variable to a constant array, and shouldn't have used the word static. In retrospect, I think the cost of this is probably just the same as zeroing it, since behind the scenes it has to be copied from a constant? But the rejection wasn't due to the performance harm, but was based on a philosophy that one shouldn't change "provably" correct just to make it more compliant.

I may be using incorrect terminology, or at least I think you are presuming a more object oriented interpretation. I don't recall the exact circumstance, but it was a case where one would normally call calloc() rather than malloc() once for a buffer. This proposal to switch to calloc() was rejected due to expense of calling bzero(). I think I proposed making it a array with an initializer rather than a pointer, taking a couple KB size penalty in return for quieting a warning and removing some code smell.

The code wasn't merely copying an uninitialized variable, it was branching on the random data that happened to be in the data returned by malloc(). This confused Valgrind, but was thought to be safe due to some later check. To me this was fragile code and unsafe practice. To the decision makers, it was a good way to save a couple cycles and a few bytes. The culture (at least at the time) felt that efficiency trumped clarity and maintainability. This is a blessing and a curse.

Reply on news.ycombinator.com