refactor: Make const references to avoid unnecessarily copying objects and enable two clang-tidy checks #25707

aureleoules · 2022-07-26T10:04:02Z

I added const references to some variables to avoid unnecessarily copying objects.

Also added two clang-tidy checks : performance-for-range-copy and performance-unnecessary-copy-initialization.

shaavan

Concept ACK

I agree with removing unused variables, as this makes the code cleaner and free of redundancies.
I verified that all the variables in the first and second commit either had their value reassigned before being used or were completely unused in their lifetime.
I verified that the variables in the third commit had their values unchanged, so it makes sense to make them a const variable.
I think merging the first and second commit would be a good idea as they are similar.
I would give the test file another view to confirm no other instance of unused variable has been left in the test file.

maflcko · 2022-07-26T11:37:24Z

src/blockfilter.cpp

@@ -169,7 +169,7 @@ const std::set<BlockFilterType>& AllBlockFilterTypes()

    static std::once_flag flag;
    std::call_once(flag, []() {
-            for (auto entry : g_filter_types) {
+            for (const auto& entry : g_filter_types) {


Those things keep coming up, so it would be good to have a clang-tidy check for this

Added in 4407213.

fanquake · 2022-07-26T12:47:20Z

test/functional/interface_zmq.py

@@ -372,7 +372,6 @@ def test_sequence(self):
        # since it greatly depends on inner-workings of blocks/mempool
        # during "deep" re-orgs. Probably should "re-construct"
        # blockchain/mempool state from notifications instead.
-        block_count = self.nodes[0].getblockcount()


Dead Python code should be getting caught by Vulture, see https://github.com/bitcoin/bitcoin/blob/master/test/lint/lint-python-dead-code.py. Maybe we need to drop the minimum confidence.

We can decrease confidence. But this line worries me a little.

Any value below 100 introduces the risk of false positives, which would create an unacceptable maintenance burden.

Can we afford false positives for the chance of catching the few missed cases?

Can we afford false positives for the chance of catching the few missed cases?

It should either be tweaked to the point that it catches obviously dead code, or just removed. No point running a dead code linter that doesn't catch dead code.

No point running a dead code linter that doesn't catch dead code.

I agree with it.

aureleoules · 2022-07-26T17:22:13Z

I've removed more unused variables and added more const references.
Also enabled two clang-tidy checks to enforce const references where applicable.

DrahtBot · 2022-07-26T17:31:34Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25872 (Fix issues when calling std::move(const&) by MarcoFalke)
#25695 (tidy: add modernize-use-using by fanquake)
#25673 (refactor: make member functions const when applicable by aureleoules)
#25429 (refactor: Avoid UniValue copies by MarcoFalke)
#24914 (wallet: Load database records in a particular order by achow101)
#23417 (wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101)
#22341 (rpc: add getxpub by Sjors)

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.

fanquake · 2022-07-27T10:56:27Z

I've removed more unused variables and added more const references.
Also enabled two clang-tidy checks to enforce const references where applicable.

In a9c0555. Don't mix refactoring Python test code & c++ networking code into the same commit. I'd actually suggest splitting the Python changes from this PR, given it's unclear if Vulture is working. That should be followed up with, and without it, it's not clear if your Python changes are exhaustive.

You should order your commits so that the changes follow logically. i.e perform refactors that allow turning on clang-tidy checks, before turning on the checks, not the other way around.

Please write commit messages that are meaningful, and explain your changes. Limit your commit titles to ~ 50 chars. (i.e 4407213).

This avoids initializing variables with the copy-constructor of a non-trivially copyable type.

aureleoules · 2022-07-27T11:34:17Z

Thank you for the reviews.

I have addressed your comments @fanquake :

dropped commit with python refactor
re-ordered and renamed commits

Though, I haven't been able to tweak vulture to detect the unused variables I found manually.

vasild

ACK 46c4dda

src/.clang-tidy

Checks enabled: 'performance-for-range-copy' and 'performance-unnecessary-copy-initialization'

vasild

ACK ae7ae36

maflcko · 2022-08-19T15:10:03Z

src/netbase.cpp

@@ -476,7 +476,7 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a
    if (recvr != IntrRecvError::OK) {
        return error("Error reading from proxy");
    }
-    if ((recvr = InterruptibleRecv(pchRet3, 2, g_socks5_recv_timeout, sock)) != IntrRecvError::OK) {
+    if (InterruptibleRecv(pchRet3, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
        return error("Error reading from proxy");
    }
    LogPrint(BCLog::NET, "SOCKS5 connected %s\n", strDest);


for reference, the changes in this file are not detected by clang-tidy, nor do they have anything to do with const, but they look correct

maflcko

review ACK ae7ae36

…sarily …

…sarily copying objects and enable two clang-tidy checks ae7ae36 tidy: Enable two clang-tidy checks (Aurèle Oulès) 081b0e5 refactor: Make const refs vars where applicable (Aurèle Oulès) Pull request description: I added const references to some variables to avoid unnecessarily copying objects. Also added two clang-tidy checks : [performance-for-range-copy](https://releases.llvm.org/11.1.0/tools/clang/tools/extra/docs/clang-tidy/checks/performance-for-range-copy.html) and [performance-unnecessary-copy-initialization](https://releases.llvm.org/12.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.html). ACKs for top commit: vasild: ACK ae7ae36 MarcoFalke: review ACK ae7ae36 Tree-SHA512: f6ac6b0cd0eee1e0c34d2f186484bc0f7ec6071451cccb33fa88a67d93d92b304e2fac378b88f087e94657745bca4e966dbc443759587400eb01b1f3061fde8c

aureleoules changed the title ~~2022 07 cleanup~~ refactor: Cleanup variables Jul 26, 2022

aureleoules changed the title ~~refactor: Cleanup variables~~ refactor: Cleanup unused variables Jul 26, 2022

DrahtBot added the Refactoring label Jul 26, 2022

shaavan reviewed Jul 26, 2022

View reviewed changes

maflcko reviewed Jul 26, 2022

View reviewed changes

fanquake reviewed Jul 26, 2022

View reviewed changes

aureleoules force-pushed the 2022-07-cleanup branch 4 times, most recently from 4403522 to 5f99c7a Compare July 26, 2022 16:06

aureleoules changed the title ~~refactor: Cleanup unused variables~~ refactor: Cleanup unused variables and enable two clang-tidy checks Jul 26, 2022

This was referenced Jul 26, 2022

refactor: Avoid UniValue copies #25429

Closed

wallet: Load database records in a particular order #24914

Merged

wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager #23417

Closed

refactor: Make const refs vars where applicable

081b0e5

This avoids initializing variables with the copy-constructor of a non-trivially copyable type.

aureleoules force-pushed the 2022-07-cleanup branch from 5f99c7a to 46c4dda Compare July 27, 2022 11:28

aureleoules changed the title ~~refactor: Cleanup unused variables and enable two clang-tidy checks~~ refactor: Make const references to avoid unnecessarily copying objects and enable two clang-tidy checks Jul 27, 2022

fanquake mentioned this pull request Jul 27, 2022

refactor: make member functions const when applicable #25673

Closed

aureleoules force-pushed the 2022-07-cleanup branch 2 times, most recently from 6d69575 to 46c4dda Compare July 27, 2022 18:22

vasild approved these changes Jul 29, 2022

View reviewed changes

src/.clang-tidy Outdated Show resolved Hide resolved

tidy: Enable two clang-tidy checks

ae7ae36

Checks enabled: 'performance-for-range-copy' and 'performance-unnecessary-copy-initialization'

aureleoules force-pushed the 2022-07-cleanup branch from 46c4dda to ae7ae36 Compare August 3, 2022 15:18

DrahtBot mentioned this pull request Aug 4, 2022

tidy: add modernize-use-using #25695

Closed

DrahtBot mentioned this pull request Aug 11, 2022

rpc: add path to gethdkey #22341

Closed

vasild approved these changes Aug 12, 2022

View reviewed changes

DrahtBot mentioned this pull request Aug 19, 2022

Fix issues when calling std::move(const&) #25872

Merged

maflcko reviewed Aug 19, 2022

View reviewed changes

maflcko merged commit 9eaef10 into bitcoin:master Aug 19, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 19, 2022

Merge bitcoin#25707: refactor: Make const references to avoid unneces…

7755559

…sarily …

aureleoules deleted the 2022-07-cleanup branch August 25, 2022 10:23

aureleoules mentioned this pull request Dec 1, 2022

test: remove unused vars in feature_block #26507

Merged

bitcoin locked and limited conversation to collaborators Aug 25, 2023

refactor: Make const references to avoid unnecessarily copying objects and enable two clang-tidy checks #25707

refactor: Make const references to avoid unnecessarily copying objects and enable two clang-tidy checks #25707

Uh oh!

Conversation

aureleoules commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

aureleoules Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

fanquake Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

shaavan Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

fanquake Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

shaavan Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

aureleoules commented Jul 26, 2022

Uh oh!

DrahtBot commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

fanquake commented Jul 27, 2022

Uh oh!

aureleoules commented Jul 27, 2022

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aureleoules commented Jul 26, 2022 •

edited

Loading

DrahtBot commented Jul 26, 2022 •

edited

Loading