build: Add Werror=range-loop-analysis #19719

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2008-buildLoopErr changing 2 files +2 −1

MarcoFalke commented at 1:29 PM on August 14, 2020: member
The warning is implicitly enabled for Bitcoin Core. Also explicitly since commit d92204c900d.

To avoid "fix range loop" follow-up refactors, we have two options:
- Disable the warning, so that issues never appear
- Enable it as an error, so that issues are either caught locally or by ci
build: Add Werror=range-loop-analysis fa55c1d5fd
MarcoFalke added the label Build system on Aug 14, 2020
jonatack commented at 1:44 PM on August 14, 2020: member

utACK, I enable this as an error locally.

in src/wallet/wallettool.cpp:133 in fa55c1d5fd

 129 | @@ -130,7 +130,7 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name)
 130 |              std::vector<bilingual_str> warnings;
 131 |              bool ret = RecoverDatabaseFile(path, error, warnings);
 132 |              if (!ret) {
 133 | -                for (const auto warning : warnings) {
 134 | +                for (const auto& warning : warnings) {

hebasto commented at 2:12 PM on August 14, 2020:

IIUC, this change prevents -Wrange-loop-analysis warning/error. But on my system (Linux Mint 20 x86_64, Ubuntu 20.04 code base, clang version 10.0.0-4ubuntu1) I cannot see a such warning.

How could i reproduce it?

hebasto commented at 4:33 PM on August 14, 2020:

Ok, I can see this warning on macOS 10.15.6 with Apple's clang (Apple LLVM version 11.0.3 (clang-1103.0.32.29)):

wallet/wallettool.cpp:133:33: warning: loop variable 'warning' of type 'const bilingual_str' creates a copy from type 'const bilingual_str' [-Wrange-loop-analysis]
                for (const auto warning : warnings) {
                                ^
wallet/wallettool.cpp:133:22: note: use reference type 'const bilingual_str &' to prevent copying
                for (const auto warning : warnings) {
                     ^~~~~~~~~~~~~~~~~~~~
1 warning generated.

hebasto commented at 4:35 PM on August 14, 2020:

I think 6afb234ee28d1635944bf25d5c7e8b4f6232d077 demo is not required :)

hebasto commented at 7:58 AM on August 15, 2020:

I was wrong: the warning is reproducible with clang version 10.0.0-4ubuntu1.

fanquake commented at 3:32 AM on August 18, 2020:

Yes there is nothing macOS specific going on here.

practicalswift commented at 3:12 PM on August 14, 2020: contributor

ACK fa55c1d5fdd88c4bc4d361da231cd63b20255b50 -- pre-review fix-up is better than post-review fix-up
hebasto approved
hebasto commented at 4:55 PM on August 14, 2020: member
ACK fa55c1d5fdd88c4bc4d361da231cd63b20255b50, tested on:
- Linux Mint 20, x86_64:
  
  clang version 10.0.0-4ubuntu1 (a warning removed)
- macOS 10.15.6:
  
  Apple LLVM version 11.0.3 (clang-1103.0.32.29) (a warning removed)
  
  brew llvm clang version 10.0.0 (no warnings on master)
Is any purpose why -Wformat, -Wformat-security, and -Wredundant-decls do not have -Werror pair?

promag commented at 11:57 PM on August 14, 2020: member

Concept ACK.

macos jobs on travis are failing with

wallet/wallettool.cpp:133:33: error: loop variable 'warning' of type 'const bilingual_str' creates a copy from type 'const bilingual_str' [-Werror,-Wrange-loop-analysis]
                for (const auto warning : warnings) {
                                ^
wallet/wallettool.cpp:133:22: note: use reference type 'const bilingual_str &' to prevent copying
                for (const auto warning : warnings) {
                     ^~~~~~~~~~~~~~~~~~~~
1 error generated.
Makefile:12108: recipe for target 'wallet/libbitcoin_wallet_tool_a-wallettool.o' failed

MarcoFalke commented at 6:37 AM on August 15, 2020: member

Failure in ci would look something like this: https://travis-ci.org/github/bitcoin/bitcoin/builds/717972341

Going to drop the last failing commit now.
MarcoFalke force-pushed on Aug 15, 2020
hebasto approved
hebasto commented at 6:39 AM on August 15, 2020: member

re-ACK fa55c1d5fdd88c4bc4d361da231cd63b20255b50
MarcoFalke commented at 6:40 AM on August 15, 2020: member

Is any purpose why -Wformat, -Wformat-security, and -Wredundant-decls do not have -Werror pair?

No idea, might be an oversight. :man_shrugging:
fanquake approved
fanquake commented at 3:33 AM on August 18, 2020: member

ACK fa55c1d5fdd88c4bc4d361da231cd63b20255b50
fanquake merged this on Aug 18, 2020
fanquake closed this on Aug 18, 2020
sidhujag referenced this in commit 69fa77c9cf on Aug 18, 2020
MarcoFalke deleted the branch on Aug 18, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels