Make ENABLE_WERROR actually work

[lhchavez]

This change explicitly adds -Werror to the CFLAGS.

Due to the way that the ADD_C_FLAG_IF_SUPPORTED() macro was mangling the
flag name to convert it into a define name, any warning that had a dash
in its name was not being correctly enabled. Additionally, any flag that
is enabled implicitly by the compiler (like -Wunused-result and
-Wdeprecated-declarations) would not cause an error unless they were
explicitly enabled with the ENABLE_WARNINGS() macro.

[ethomson]

Hmm, it looks like this only works for GCC-like compilers. I wonder if we might also enable warnings on errors in MSVC? (/WX, if I recall correctly).

[ethomson]

I'd also like @pks-t's thoughts here. ENABLE_WERROR currently upgrades the explicit warnings that we've defined to be errors, it isn't a blanked -Werror. The former option sounds like it remains useful, and that we might want to consider both.

Perhaps a -DENABLE_WERROR=all for the latter?

[lhchavez]

hmm i thought that was the intention, given that -Werror=all and -Werror=extra are enabled here:

libgit2/CMakeLists.txt

Lines 201 to 202 in 274a37f

	ENABLE_WARNINGS(all)
	ENABLE_WARNINGS(extra)

enabling warnings on MSVC is going to take more effort, since I don't have a Windows machine and debugging on Travis is hard ^^;;

[lhchavez]

actually let me split off the non-controversial part of this change to a separate PR.

[lhchavez]

Done. Moved the fix of the warnings to #4928 , so this will break the Travis build until that one is merged.

[ethomson]

FYI, we don't use Travis. :)

[ethomson]

hmm i thought that was the intention, given that -Werror=all and -Werror=extra are enabled here:

Yeah, I can't speak to that, as I don't think that I understand the distinction between -Werror=all and -Wall -Werror. It seems like it was very explicit to do -Werror=all instead of -Wall -Werror.

[ethomson]

In any case, don't worry about MSVC, I can tackle that once we've decided the direction that we want to go in.

[neithernut]

Yeah, I can't speak to that, as I don't think that I understand the distinction between -Werror=all and -Wall -Werror. It seems like it was very explicit to do -Werror=all instead of -Wall -Werror.

If I understand correctly, -Wall -Werror causes all warnings to be considered errors, including warnings enabled by other switches (e.g. -Wextra). Werror=all causes only those warnings enabled by -Wall to be considered errors.

[ethomson]

If I understand correctly, -Wall -Werror causes all warnings to be considered errors, including warnings enabled by other switches (e.g. -Wextra). Werror=all causes only those warnings enabled by -Wall to be considered errors.

That's my understanding, too. Sorry, for additional context, what we do when ENABLE_WERROR is turned on is basically go through the warnings that we enable and append =error to them.

In other words, instead of -Wall -Wextra -Wdocumentation we'll have -Wall=error -Wextra=error -Wdocumentation=error.

This feels like it should be the equivalent of -Wall -Wextra -Wdocumentation -Werror but it appears that it is not. It feels like there's some warnings implicitly enabled (and then promoted to errors) by -Werror?

I'm definitely not understand the distinction yet. 😀

[lhchavez]

yes, the two flags that i had trouble with ( -Wmaybe-uninitialized -Wdeprecated-declarations) are implicitly enabled and not covered by -Wall or -Werror. an equivalent change would be to explicitly include all implicit warnings, but that seems subpar.

[lhchavez]

All right, after a lot of debugging, I finally traced the root of the issue to the way the ADD_C_FLAG_IF_SUPPORTED() macro was being expanded.

Warning-free builds FTW!

[tiennou]

LGTM, thanks for tackling this @lhchavez !

[ethomson]

Cool, thanks!

[pks-t]

For some background: -Wall -Werror does not turn all warnings into errors. I had this at some point in the past, and it doesn't turn "additional" warnings into errors. So e.g. if I had -Wformat, then those warnings wouldn't have been converted to an error. This is why I had to be explicit and always say -Werror=<option>.

[pks-t]

So yeah, I tried my luck to reproduce the old behaviour I was seeing, but right now I can't. I'm not quite happy that we now pass -Werror for every single warning (even the disabled ones), buy anyway. Let's see whether I'll accidentally reproduce the remembered behaviour somewhen.

[lhchavez]

Hmm that is very odd. I know that most -Wxx/-Wno-xxx flags depend on the order in which they are added to the commandline, but -Werror is a big exception to that so no matter where you put it in the commandline, any not-explicitly-disabled warning is upgraded to an error (unless you also add -Wno-error=xxx, which allows a warning to not be upgraded to an error).

maybe there was an ordering issue with -Wformat?