refactor: Remove redundant definitions

Empact commented at 5:08 pm on February 27, 2024: contributor

These were identified via -Wshadow in #29485, see the commit messages for the associated compiler message.

DrahtBot commented at 5:08 pm on February 27, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29492.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	maflcko

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Refactoring on Feb 27, 2024

in src/common/init.cpp:73 in b1fe46a4cd outdated

69@@ -70,7 +70,7 @@ std::optional<ConfigError> InitConfig(ArgsManager& args, SettingsAbortFn setting
70             const std::string config_source = cli_config_path.empty()
71                 ? strprintf("data directory %s", fs::quoted(fs::PathToString(orig_datadir_path)))
72                 : strprintf("command line argument %s", fs::quoted("-conf=" + cli_config_path));
73-            const std::string error = strprintf(
74+            error = strprintf(

maflcko commented at 5:28 pm on February 27, 2024:

not sure. Shadowing is a side effect of the effort to limit the scope of a symbol to a smaller scope. I don’t think re-using a symbol of the outer scope is the correct solution, merely because it happens to have the same name and type.

Empact force-pushed on Feb 27, 2024

Empact commented at 11:11 pm on February 27, 2024: contributor

My inclination would be to fix the -Wshadow warnings over the course of several PRs (to ease review/minimize conflicts), then add the warning to one of the CI runs to guard against introduction, while leaving it disabled by default to avoid other incidental disruption.

DrahtBot added the label CI failed on Apr 19, 2024

DrahtBot removed the label CI failed on Apr 23, 2024

achow101 commented at 2:37 pm on October 15, 2024: member

Are you still working on this?

in src/net_processing.cpp:3072 in da188bf523 outdated

3036@@ -3037,8 +3037,6 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
3037     AssertLockHeld(g_msgproc_mutex);
3038     LOCK(cs_main);
3039 
3040-    CTransactionRef porphanTx = nullptr;

maflcko commented at 2:39 pm on October 15, 2024:

I wonder why clang-tidy can’t pick this one up. Ref: https://github.com/bitcoin/bitcoin/pull/31051/files

fanquake commented at 6:38 pm on February 20, 2025:

Maybe we can circle back with a future version.

DrahtBot added the label Needs rebase on Oct 29, 2024

refactor: Don't redundantly define error in common/init.cpp

common/init.cpp:73:31: error: declaration shadows a local variable [-Werror,-Wshadow]
            const std::string error = strprintf(
                              ^
common/init.cpp:37:21: note: previous declaration is here
        std::string error;
                    ^

c17de5429e

refactor: Don't redundantly define porphanTx in net_processing.cpp

net_processing.cpp:3042:28: error: declaration shadows a local variable [-Werror,-Wshadow]
    while (CTransactionRef porphanTx = m_orphanage.GetTxToReconsider(peer.m_id)) {
                           ^
net_processing.cpp:3040:21: note: previous declaration is here
    CTransactionRef porphanTx = nullptr;
                    ^

f953bf4d99

refactor: Don't redundantly define TxItems in wallet/wallet.cpp

wallet/wallet.cpp:901:48: error: declaration shadows a typedef in 'wallet::CWallet' [-Werror,-Wshadow]
    typedef std::multimap<int64_t, CWalletTx*> TxItems;
                                               ^
./wallet/wallet.h:483:48: note: previous declaration is here
    typedef std::multimap<int64_t, CWalletTx*> TxItems;
                                               ^

b267c98c6c

test: Don't redundantly define msg in key_tests

test/key_tests.cpp:184:21: error: declaration shadows a local variable [-Werror,-Wshadow]
        std::string msg = "A message to be signed" + ToString(i);
                    ^
test/key_tests.cpp:161:17: note: previous declaration is here
    std::string msg = "A message to be signed";
                ^

6d300e84d4

Empact force-pushed on Jan 26, 2025

DrahtBot removed the label Needs rebase on Jan 26, 2025

DrahtBot added the label CI failed on Jan 26, 2025

DrahtBot removed the label CI failed on Feb 3, 2025

maflcko commented at 3:55 pm on February 14, 2025: member

tend towards NACK, without a motivation (example of a real bug that this prevents or prevented):

Given a motivation, it should be enforced with clang-tidy (or otherwise)
Without motivation, it shouldn’t be enforced, nor should the changes be made in the first place

fanquake commented at 6:38 pm on February 20, 2025: member

Closing for now.

fanquake closed this on Feb 20, 2025

refactor: Remove redundant definitions #29492

Code Coverage & Benchmarks

Reviews

Conflicts