Set Certain Warnings as Errors Unconditionally #890
Conversation
vector-of-bool
requested review from
rcsanchez97,
kevinAlbs,
chardan and
eramongodb
build/cmake/MongoC-Warnings.cmake
Outdated
| # Missing return types/statements | ||
| gnu-like:-Werror=return-type msvc:/we4716 | ||
| # Incompatible pointer types | ||
| gnu-like:$<${is_c_lang}:-Werror=incompatible-pointer-types> msvc:/we4113 |
There was a problem hiding this comment.
This one might be problematic on older compilers with the new atomics. For instance, here is an excerpt of build output from RHEL 6.2 (gcc 4.4.7):
/root/mongo-c-driver.git/src/libbson/src/bson/bson-atomic.c: In function 'bson_atomic_int_add':
/root/mongo-c-driver.git/src/libbson/src/bson/bson-atomic.c:28: warning: passing argument 1 of 'bson_atomic_int32_fetch_add' from incompatible pointer type
/root/mongo-c-driver.git/src/libbson/src/bson/bson-atomic.h:323: note: expected 'volatile long int *' but argument is of type 'volatile int32_t *'
/root/mongo-c-driver.git/src/libbson/src/bson/bson-atomic.c: In function '_lock_64bit_atomic':
/root/mongo-c-driver.git/src/libbson/src/bson/bson-atomic.c:61: warning: passing argument 1 of 'bson_atomic_int_compare_exchange_weak' from incompatible pointer type
/root/mongo-c-driver.git/src/libbson/src/bson/bson-atomic.h:324: note: expected 'volatile long int *' but argument is of type 'int *'
We might want to wait on this PR until after CDRIVER-4215 is done, as that might clear some of the warnings which would turn into errors with this change.
There was a problem hiding this comment.
There's another open ticket about some related implicit conversions in the atomics code. I plan to inspect it, because it is very suspicious.
There was a problem hiding this comment.
+1. I think these will generally only improve the code, although as has been pointed out we should check how they behave across all the compiler versions we support; I'm less familiar with flags on the Microsoft compiler, but from what I've read I think it's also ok.
| @@ -10,7 +10,6 @@ | |||
| -Wno-strict-aliasing | |||
| -Wno-uninitialized | |||
| -Wredundant-decls | |||
| -Wreturn-type | |||
There was a problem hiding this comment.
I'm curious as to why we're removing this one, it seems to fall into the "almost always an error" category?
There was a problem hiding this comment.
It was moved into the MongoC-Warnings.cmake file. This just removes the redundancy.
build/cmake/MongoC-Warnings.cmake
Outdated
| # Incompatible pointer types | ||
| gnu-like:$<${is_c_lang}:-Werror=incompatible-pointer-types> msvc:/we4113 | ||
| # Integral/pointer conversions | ||
| gnu-like:$<${is_c_lang}:-Werror=int-conversion> msvc:/we4047 |
There was a problem hiding this comment.
I've seen some (usually pretty old) code where this can make things noisy-- but it is indeed "almost always an error".
|
@vector-of-bool Now that #889 is merged, you might consider rebasing these changes and starting a new patch build. It will give us useful information about the effect of these changes on the build for older platforms. |
There was a problem hiding this comment.
The workaround for CMake versions less than 3.3 (assuming C language even for C++ targets) leaves me with an unsatisfied feeling. I feel the situation could be improved by using target_compile_options rather than add_compile_options, but I do not consider it to be a blocker for this PR.
There was a problem hiding this comment.
Nice work! The generator expressions make it simpler to declare equivalent warnings across compilers.
I suggest running a full patch build to check all compilers on the Evergreen waterfall. That can be done with the Evergreen CLI: evergreen patch --project=mongo-c-driver
build/cmake/MongoC-Warnings.cmake
Outdated
|
|
||
| #[[ | ||
| Define additional compile options, conditional on the compiler being used. | ||
| Each option should be prefixed by `gnu-like:` or `msvc:`. Those options will be |
There was a problem hiding this comment.
The logic of the function allows for prefixes of gnu-like, gnu, clang, and msvc. Perhaps this function header comment should be updated for consistency.
There was a problem hiding this comment.
I forgot to update this comment, yeah.
Warning became a hard error following #890. Mark the offending code path as unreachable.
Some constructs that are allowed in C are well-established to be virtually always erroneous, e.g. calling a function that hasn't been declared, implicitly binding a pointer to a different type, and implicitly converting an integral to a pointer. Implicitly converting a pointer-to-
constto a pointer-to-mutableis not always an error, but is problematic enough that an explicit cast should be required.Many of these errors will be caught at a later time, but only due to test failures, linker failures, or on a build on CI that enables some warnings-as-errors (currently never on Windows).
This change instead makes such constructs always-errors, thus adding a barrier to any changes that produce such errors that might otherwise have been missed. The selected warnings are well-established and detected by all modern compilers, and are very unlikely to appear anew do to compiler upgrades.
Additional warnings that are not so definitely-wrong (etc. sign conversion/compare, unused variables) are not addressed by these changes.