search

apple / foundationdb

FoundationDB - the open source, distributed, transactional key-value store
https://apple.github.io/foundationdb/
Apache License 2.0
14.58k stars 1.32k forks source link

Get rid of compiler warnings (paying technical debt) #1255

Open mpilman opened 5 years ago

mpilman commented 5 years ago

Under Linux+OS X we don't have any compilation warnings. But the reason for that is simply because I chose the compilation flags in a way that suppresses these warnings. However, some of these warnings would be very useful. This can be seen on Windows: compilation there generates thousands of warnings.

Not having these useful warnings enabled is bothering me: every bug the compiler can find is a bug I don't have to track down in a simulation run.

The obvious problem with enabling more warnings is, that they will just be ignored as it will be too overwhelming seeing so many of those. I would suggest the following strategy:

  1. We should come up with a number of warnings we want to enable in gcc/clang/vsc
  2. Each Apple/Snowflake developer takes one or two of these flags, enables it on their fork and fixes all occurrences. I would not be surprised, if this would unearth a few bugs.
  3. Enable all these warnings by default and compile with -Werror
  4. Celebrate the fact that we will have a cleaner code-base and that the compiler will find more bugs for us.
atn34 commented 5 years ago

@AlvinMooreSr, @mpilman, and I discussed this recently and have a plan for moving forward on this issue.

The plan is to compile without -Werror by default so that we limit the scope of enabling compiler warnings. CI will enforce that PRs compile successfully with -Werror in the official docker container.

Adding more environments where we're committed to compiling with -Werror will be a longer term goal.

Also, this issue needs an assignee. I'm happy to own it.

atn34 commented 5 years ago

-Wmaybe-uninitialized looks like a good candidate for the next warning to enable

atn34 commented 4 years ago

This does not have clear criteria for being closed by 6.3, so I'm un-milestoning it.