refactor: Remove redundant definitions #29492

pull Empact wants to merge 4 commits into bitcoin:master from Empact:redundant-definitions changing 4 files +5 −10
  1. 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.

    <!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

    <!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

  2. DrahtBot commented at 5:08 PM on February 27, 2024: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Refactoring on Feb 27, 2024
  4. 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.

  5. Empact force-pushed on Feb 27, 2024
  6. 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.

  7. DrahtBot added the label CI failed on Apr 19, 2024
  8. DrahtBot removed the label CI failed on Apr 23, 2024
  9. achow101 commented at 2:37 PM on October 15, 2024: member

    Are you still working on this?

  10. 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.

  11. DrahtBot added the label Needs rebase on Oct 29, 2024
  12. 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
  13. 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
  14. 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
  15. 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
  16. Empact force-pushed on Jan 26, 2025
  17. DrahtBot removed the label Needs rebase on Jan 26, 2025
  18. DrahtBot added the label CI failed on Jan 26, 2025
  19. DrahtBot removed the label CI failed on Feb 3, 2025
  20. 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
  21. fanquake commented at 6:38 PM on February 20, 2025: member

    Closing for now.

  22. fanquake closed this on Feb 20, 2025
Contributors
EmpactDrahtBotmaflckoachow101fanquake
Labels
Refactoring
Linked (view graph)
#29485 build: Enable -Wshadow
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-22 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me