BUILD: Use -Werror=return-type (and MSVC equivalent)#6570
Merged
bluegr merged 1 commit intoscummvm:masterfrom Apr 25, 2025
Merged
Conversation
…statement
Regarding the `configure` script, we now have:
* default: add `-Werror=flag-name` for selected warnings where it's
important to fail early (just `-Werror=return-type` for now)
* --enable-Werror: use `-Werror` for all warnings
* --disable-Werror: don't any `-Werror` flag
For Xcode, `-Werror=return-type` is added to default flags.
For MSVC, equivalent flags have been added to the existing
`globalErrors` list.
Suggested by sluicebox.
Member
|
Nice work! It builds cleanly as well Thanks, merging |
dwatteau
deleted the
feat/build-gcc-clang-promote-selected-warnings-to-errors
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on @sluicebox's comment on commit 599fbc2, where a missing
returnstatement was caught by a fatal error in MSVC (thanks to previous PR #4561) but not in the GCC/Clang builds.This PR now makes the three main builders (
configure, MSVC, Xcode) have such a behavior, for this specific case.configurechangesFor the
configurescripts, the new flag is set by default, but one can turn it off with--disable-Werror, if some situation requires it (e.g. some Linux distributions having a good reason to do so, some future compiler upgrade introducing a stricter/buggier-Wreturn-type…).(Regarding
CXXFLAGS: I'm not usingset_flag_if_supported, since-Werror=flag-namewas introduced before GCC 4.7, which is our minimal C++11 requirement.)Of course, turning a specific warning into a fatal error should only be done when it reveals a dangerous pattern where we want to fail early (to be sure to catch it), and if the compilers don't create frequent false positives for it.
False positives?
Regarding false positives, @lephilousophe mentioned godotengine/godot#58747.
I also saw https://reviews.llvm.org/D98224 mentioning that "some versions of gcc can emit
-Wreturn-typewarnings on functions that have exhaustive switches over enums" (this seems to be related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87950, where__builtin_unreachable()is also mentioned).Anyway, I did some
--enable-all-enginestests with:./devtools/docker.sh toolchains/riscos(our de-facto oldest toolchain? GCC 4.7)./devtools/docker.sh toolchains/windows-9x./devtools/docker.sh toolchains/ps3./devtools/docker.sh toolchains/windows-9xand I couldn't find any false-positive case.