Skip to content

refactor: Remove redundant definitions #29492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

refactor: Remove redundant definitions #29492

wants to merge 4 commits into from

Conversation

Empact
Copy link
Contributor

@Empact Empact commented Feb 27, 2024

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 27, 2024 •
edited
Loading

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.

maflcko
maflcko reviewed Feb 27, 2024
View reviewed changes
@Empact Empact force-pushed the redundant-definitions branch from b1fe46a to da188bf Compare February 27, 2024 17:50
@Empact
Copy link
Contributor Author

Empact commented Feb 27, 2024

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 DrahtBot mentioned this pull request Feb 28, 2024
Closed
@DrahtBot DrahtBot mentioned this pull request May 23, 2024
Merged
@DrahtBot DrahtBot mentioned this pull request Sep 5, 2024
Closed
@DrahtBot DrahtBot mentioned this pull request Oct 11, 2024
Closed
@achow101
Copy link
Member

Are you still working on this?

maflcko
maflcko reviewed Oct 15, 2024
View reviewed changes
src/net_processing.cpp
@@ -3037,8 +3037,6 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
AssertLockHeld(g_msgproc_mutex);
LOCK(cs_main);

CTransactionRef porphanTx = nullptr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can circle back with a future version.

Empact added 4 commits January 26, 2025 01:17
@Empact
refactor: Don't redundantly define error in common/init.cpp
c17de54 
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;
                    ^
@Empact
refactor: Don't redundantly define porphanTx in net_processing.cpp
f953bf4 
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;
                    ^
@Empact
refactor: Don't redundantly define TxItems in wallet/wallet.cpp
b267c98 
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;
                                               ^
@Empact
test: Don't redundantly define msg in key_tests
6d300e8 
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";
                ^
@Empact Empact force-pushed the redundant-definitions branch from da188bf to 6d300e8 Compare January 26, 2025 09:18
@DrahtBot DrahtBot removed the CI failed label Feb 3, 2025
@maflcko
Copy link
Member

maflcko commented Feb 14, 2025

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
Copy link
Member

Closing for now.

@fanquake fanquake closed this Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fanquake fanquake fanquake left review comments

+1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Empact @DrahtBot @achow101 @maflcko @fanquake