build: Add -Werror=missing-noreturn #21667
Conversation
This change allows to apply more -W and -Werror options to our own code.
ff19e5b to
9b50b23
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
Additionally, it makes --enable-suppress-external-warnings the default configure options,
I'm not sure why this is mentioned as a second line change, as this is the major change here. Also, if the defaults are being changed, shouldn't some of the configure logic be reversed to only apply certain warnings in the now, non-default case? For now, I'm an approach NACK.
It would be great if you could summarize (maybe in a new issue or gist) what the end goals are with some of your current build system changes:
- Where are you trying to get our build system to?
- What would you envision our "perfect" build system setup to be?
- How will/should release builds differ from "regular" builds vs debugging builds vs building in the CI etc.
- Taking into account system packages vs depends packages
- What are the expectations for when warnings are "allowed" vs when they should be suppressed?
- When should they be disabled/tested in configure, vs patch around / pragma'd away?
- When is it "ok" to turn on something that might be broken in an older compiler etc.
- Are you expecting us to be building with
-Werrorby default at some point?- This is unlikely to be the case.
- What are the missing additional checks / warnings that you think would be very valuable to apply to our codebase?
At the moment it seems like a very inconsistent/whack-a-mole type approach is being taken, mostly just to suppress warnings (sometimes at any cost. i.e #20824), but also sometimes disabling checks outright, or patching around them in our own code.
| @@ -431,17 +431,18 @@ if test "x$enable_werror" = "xyes"; then | |||
| AX_CHECK_COMPILE_FLAG([-Werror=unused-variable],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=unused-variable"],,[[$CXXFLAG_WERROR]]) | |||
| AX_CHECK_COMPILE_FLAG([-Werror=date-time],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=date-time"],,[[$CXXFLAG_WERROR]]) | |||
| AX_CHECK_COMPILE_FLAG([-Werror=return-type],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=return-type"],,[[$CXXFLAG_WERROR]]) | |||
| AX_CHECK_COMPILE_FLAG([-Werror=conditional-uninitialized],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=conditional-uninitialized"],,[[$CXXFLAG_WERROR]]) | |||
There was a problem hiding this comment.
In ebe68bd. I don't understand why a number of checks are being moved in this commit (no explanation in the commit message). Isn't this reducing the scope of when they are applied? If they work fine now (no warnings), why do they need to be moved inside the scope suppressing external warnings (and only after you've suppressed warnings from leveldb)?
|
I don't think we should make |
|
🕵️ @achow101 @sipa @practicalswift have been requested to review this pull request as specified in the REVIEWERS file. |
|
This PR requires #22041. So, closing for now. |
This PR is a #21633 follow up.
Additionally, it makes
--enable-suppress-external-warningsthe defaultconfigureoptions, that allows to apply more-Wand-Werroroptions to our own code (e.g., #21613).