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

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

I think there is still a benefit to developers being notified about warnings in headers. Otherwise no one may notice the false warnings and report them upstream. Also, if they point to real problems, it will be harder to track them down.

I think there is still a benefit to developers being notified about warnings in headers.

They can still be notified, by disabling the suppression.

Otherwise no one may notice the false warnings and report them upstream.

Then we should stop suppressing them globally in the CI, so we are more likely to see (and then report) the warnings across all relevant platforms / arches. Although then we can't use -Werror (which is also why some devs would always be suppressing locally).

Also, it may be good to give some more background. IIRC this is only needed for bdb4 and qt5, so someone compiling bitcoind with sqlite shouldn't have to suppress warnings, no?

IIRC this is only needed for bdb4 and qt5,

There are likely warnings produced for all dependencies (includeing Boost and libevent),
because warning output is dependant on:

• architecture
• operating system (& version)
• compiler (& version)
• compiler flags being used
• other compile options i.e LTO
• packages being used (& version) (& system vs depends)

Where a number of the above continue to change over time.

so someone compiling bitcoind with sqlite shouldn't have to suppress warnings, no?

This isn't the case, for example, for someone compiling on macOS, with only sqlite. They'll see warnings for both Boost and libevent. Someone cross-compiling for riscv64 on Ubuntu will see warnings from Boost (Test) etc.

so someone compiling bitcoind with sqlite shouldn't have to suppress warnings, no?

I get a large number of warnings from boost.

Concept ACK. #22041 :)

I think this comment by @Sjors:

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.

is still relevant. Maybe its "should be better documented" part is worth being added to this PR?

"should be better documented"

How? Where?

"should be better documented"

How? Where?

Developer Notes? Maybe in the "Development tips and tricks" section?

cc @Sjors

Did this recently annoy you, or were there users opening issues / having trouble parsing through the warnings?

Concept ACK

I don't ever run configure without this flag, so having it out of mind is a win for me.

Did this recently annoy you, or were there users opening issues / having trouble parsing through the warnings?

On Ubuntu 22.04:

$ ./autogen.sh && ./configure CC=clang CXX=clang++
$ make 2> >(wc -l >&2) > /dev/null 
168634

It does not annoy me at all :)

It just makes spotting of warnings I am interesting in a bit harder.

Drop it as well?

I think it's correct as-is?

Drop it as well?

I think it's correct as-is?

Yes. Technically, it is correct. But it seems confusing to me as the line "The output is denoised of errors from external dependencies and includes with --enable-suppress-external-warnings and --config src/.bear-tidy-config. Both options may be omitted to view the full list of errors." explains the options used the followed code snippet. And the --enable-suppress-external-warnings option is not used in there anymore.

Ok. Just deleted all that.

ACK 3b2acfc