Skip to content
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.

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

test: Avoid overwriting the NodeContext member of the testing setup [-Wshadow-field] #19188

Merged
merged 2 commits into from Jun 8, 2020

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jun 6, 2020

Adding this warning will eliminate unexpected test failures and hard to review code. Moreover, there shouldn't be a use case in Bitcoin Core that relies on fields to be shadowed.

@maflcko
Copy link
Member Author

maflcko commented Jun 6, 2020

@maflcko maflcko changed the title 2006 test no shadow field test: Avoid overwriting the NodeContext member of the testing setup Jun 6, 2020
@maflcko maflcko changed the title test: Avoid overwriting the NodeContext member of the testing setup test: Avoid overwriting the NodeContext member of the testing setup [-Wshadow-field] Jun 6, 2020
@maflcko
Copy link
Member Author

maflcko commented Jun 6, 2020

Can be tested by reverting the fix and observing the warning:

In file included from wallet/test/init_test_fixture.cpp:8:
./wallet/test/init_test_fixture.h:21:17: warning: non-static data member 'm_node' of 'InitWalletDirTestingSetup' shadows member inherited from type 'BasicTestingSetup' [-Wshadow-field]
    NodeContext m_node;
                ^
./test/util/setup_common.h:76:17: note: declared here
    NodeContext m_node;
                ^
1 warning generated.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 6, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@hebasto
Copy link
Member

hebasto commented Jun 6, 2020

Concept ACK.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK fac6b9b, tested on Linux Mint 19.3 (x86_64):

  • Clang 9.0.0 accepts -Wshadow-field and works as expected
  • GCC 7.5.0 does not accept -Wshadow-field

The wider -Wshadow was disabled in #10136.

@practicalswift
Copy link
Contributor

ACK fac6b9b -- patch looks correct

@maflcko maflcko requested a review from fanquake June 7, 2020 10:46
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK fac6b9b - Warnings compiling fa16e78 are below. No warnings with fac6b9b. The -Wshadow-field diagnostic has been available in Clang since 5.0.0. It's not available for GCC.

In file included from wallet/test/wallet_test_fixture.cpp:5:
./wallet/test/wallet_test_fixture.h:22:17: warning: non-static data member 'm_node' of 'WalletTestingSetup' shadows member inherited from type 'BasicTestingSetup' [-Wshadow-field]
    NodeContext m_node;
                ^
./test/util/setup_common.h:76:17: note: declared here
    NodeContext m_node;
                ^
1 warning generated.
....
In file included from wallet/test/init_test_fixture.cpp:8:
./wallet/test/init_test_fixture.h:21:17: warning: non-static data member 'm_node' of 'InitWalletDirTestingSetup' shadows member inherited from type 'BasicTestingSetup' [-Wshadow-field]
    NodeContext m_node;
                ^
./test/util/setup_common.h:76:17: note: declared here
    NodeContext m_node;
                ^
1 warning generated.
...
In file included from wallet/test/wallet_test_fixture.cpp:5:
./wallet/test/wallet_test_fixture.h:22:17: warning: non-static data member 'm_node' of 'WalletTestingSetup' shadows member inherited from type 'BasicTestingSetup' [-Wshadow-field]
    NodeContext m_node;
                ^
./test/util/setup_common.h:76:17: note: declared here
    NodeContext m_node;
                ^
1 warning generated.
...
In file included from wallet/test/init_tests.cpp:11:
./wallet/test/init_test_fixture.h:21:17: warning: non-static data member 'm_node' of 'InitWalletDirTestingSetup' shadows member inherited from type 'BasicTestingSetup' [-Wshadow-field]
    NodeContext m_node;
                ^
./test/util/setup_common.h:76:17: note: declared here
    NodeContext m_node;
                ^
1 warning generated.
  CXX      test/util/libtest_util_a-mining.o
  CXX      test/util/libtest_util_a-net.o
In file included from wallet/test/psbt_wallet_tests.cpp:12:
./wallet/test/wallet_test_fixture.h:22:17: warning: non-static data member 'm_node' of 'WalletTestingSetup' shadows member inherited from type 'BasicTestingSetup' [-Wshadow-field]
    NodeContext m_node;
                ^
./test/util/setup_common.h:76:17: note: declared here
    NodeContext m_node;
                ^
1 warning generated.
  CXX      test/util/libtest_util_a-setup_common.o
  CXX      test/util/libtest_util_a-str.o
  CXX      test/util/libtest_util_a-transaction_utils.o
  CXX      test/util/libtest_util_a-wallet.o
  CXX      bench/bench_bitcoin-addrman.o
  CXX      bench/bench_bitcoin-bench_bitcoin.o
  CXX      bench/bench_bitcoin-bench.o
  CXX      bench/bench_bitcoin-block_assemble.o
  CXX      bench/bench_bitcoin-checkblock.o
In file included from wallet/test/coinselector_tests.cpp:12:
./wallet/test/wallet_test_fixture.h:22:17: warning: non-static data member 'm_node' of 'WalletTestingSetup' shadows member inherited from type 'BasicTestingSetup' [-Wshadow-field]
    NodeContext m_node;
                ^
./test/util/setup_common.h:76:17: note: declared here
    NodeContext m_node;
                ^
1 warning generated.
  CXX      bench/bench_bitcoin-checkqueue.o
In file included from wallet/test/wallet_tests.cpp:22:
./wallet/test/wallet_test_fixture.h:22:17: warning: non-static data member 'm_node' of 'WalletTestingSetup' shadows member inherited from type 'BasicTestingSetup' [-Wshadow-field]
    NodeContext m_node;
                ^
./test/util/setup_common.h:76:17: note: declared here
    NodeContext m_node;
                ^
wallet/test/wallet_tests.cpp:538:17: warning: non-static data member 'm_node' of 'ListCoinsTestingSetup' shadows member inherited from type 'BasicTestingSetup' [-Wshadow-field]
    NodeContext m_node;
                ^
./test/util/setup_common.h:76:17: note: declared here
    NodeContext m_node;
                ^
2 warnings generated.

@fanquake fanquake merged commit 807b9f8 into bitcoin:master Jun 8, 2020
@maflcko maflcko deleted the 2006-testNoShadowField branch June 8, 2020 12:15
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jul 8, 2020
…e testing setup [-Wshadow-field]

Summary:
~~NodeContext m_node is already a member of BasicTestingSetup which WalletTestingSetup inherits from via TestingSetup~~

Turns out Core had done this

Backport of Core [[bitcoin/bitcoin#19188 | PR19188]]

Test Plan:
  ninja check-all

Reviewers: #bitcoin_abc, Fabien, deadalnix

Reviewed By: #bitcoin_abc, Fabien, deadalnix

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D6835
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants