build: Add -Werror=missing-noreturn

hebasto commented at 2:20 pm on April 13, 2021: member

This PR is a #21633 follow up.

Additionally, it makes --enable-suppress-external-warnings the default configure options, that allows to apply more -W and -Werror options to our own code (e.g., #21613).

hebasto added the label Build system on Apr 13, 2021

build: In configure --enable-suppress-external-warnings by default

This change allows to apply more -W and -Werror options to our own code.

78efecb27c

build: Extend --enable-suppress-external-warnings on LevelDB ebe68bd4c5

build: Add -Werror=missing-noreturn 9b50b23214

hebasto force-pushed on Apr 13, 2021

DrahtBot commented at 11:56 pm on April 13, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22041 (build: Make –enable-suppress-external-warnings the default by hebasto)
#20586 (Fix Windows build with –enable-werror by hebasto)
#15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)

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.

in configure.ac:434 in 9b50b23214

430@@ -431,17 +431,18 @@ if test "x$enable_werror" = "xyes"; then
431   AX_CHECK_COMPILE_FLAG([-Werror=unused-variable],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=unused-variable"],,[[$CXXFLAG_WERROR]])
432   AX_CHECK_COMPILE_FLAG([-Werror=date-time],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=date-time"],,[[$CXXFLAG_WERROR]])
433   AX_CHECK_COMPILE_FLAG([-Werror=return-type],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=return-type"],,[[$CXXFLAG_WERROR]])
434-  AX_CHECK_COMPILE_FLAG([-Werror=conditional-uninitialized],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=conditional-uninitialized"],,[[$CXXFLAG_WERROR]])

fanquake commented at 3:21 am on April 14, 2021:

In ebe68bd4c5ca030ec601f9bd1e8ca20cf8d9356b. 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)?

fanquake commented at 4:16 am on April 14, 2021: member

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 -Werror by 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.

Sjors commented at 7:25 am on April 15, 2021: member

I don’t think we should make --enable-suppress-external-warnings the default, but it should be better documented. Someone needs to be motivated to fix things upstream and/or notice problems when we update dependencies.

DrahtBot commented at 9:32 am on May 3, 2021: member

🕵️ @achow101 @sipa @practicalswift have been requested to review this pull request as specified in the REVIEWERS file.

hebasto commented at 11:22 am on May 24, 2021: member

@fanquake

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 her

Right. This major change separated into #22041.

This PR is converted to a draft for now.

hebasto marked this as a draft on May 24, 2021

hebasto commented at 7:22 pm on May 29, 2021: member

This PR requires #22041.

So, closing for now.

hebasto closed this on May 29, 2021

DrahtBot locked this on Aug 18, 2022

build: Add -Werror=missing-noreturn #21667

Conflicts