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

aureleoules commented at 10:04 am on July 26, 2022: member

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.

aureleoules renamed this:
~~2022 07 cleanup~~
refactor: Cleanup variables
on Jul 26, 2022

aureleoules renamed this:
~~refactor: Cleanup variables~~
refactor: Cleanup unused variables
on Jul 26, 2022

DrahtBot added the label Refactoring on Jul 26, 2022

shaavan commented at 11:25 am on July 26, 2022: contributor

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.

in src/blockfilter.cpp:172 in 52c86f3998 outdated

168@@ -169,7 +169,7 @@ const std::set<BlockFilterType>& AllBlockFilterTypes()
169 
170     static std::once_flag flag;
171     std::call_once(flag, []() {
172-            for (auto entry : g_filter_types) {
173+            for (const auto& entry : g_filter_types) {

MarcoFalke commented at 11:37 am on July 26, 2022:

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

aureleoules commented at 4:07 pm on July 26, 2022:

Added in 4407213ca10a3a28f253fb3e14e28940144f78ce.

in test/functional/interface_zmq.py:375 in 52c86f3998 outdated

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

fanquake commented at 12:47 pm on July 26, 2022:

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.

shaavan commented at 12:59 pm on July 26, 2022:

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?

fanquake commented at 1:24 pm on July 26, 2022:

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.

shaavan commented at 2:02 pm on July 26, 2022:

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

I agree with it.

aureleoules force-pushed on Jul 26, 2022

aureleoules renamed this:
~~refactor: Cleanup unused variables~~
refactor: Cleanup unused variables and enable two clang-tidy checks
on Jul 26, 2022

aureleoules commented at 5:22 pm on July 26, 2022: member

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

DrahtBot commented at 5:31 pm on July 26, 2022: contributor

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 commented at 10:56 am on July 27, 2022: member

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

In a9c05557d527f5266fa6dfd31ad7d22bcf06c3a8. 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 4407213ca10a3a28f253fb3e14e28940144f78ce).

refactor: Make const refs vars where applicable

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

081b0e53e3

aureleoules force-pushed on Jul 27, 2022

aureleoules renamed this:
~~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
on Jul 27, 2022

aureleoules commented at 11:34 am on July 27, 2022: member

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.

aureleoules force-pushed on Jul 27, 2022

in src/.clang-tidy:8 in 46c4ddabb1 outdated

 5@@ -6,6 +6,8 @@ modernize-use-default-member-init,
 6 modernize-use-nullptr,
 7 readability-redundant-declaration,
 8 readability-redundant-string-init,
 9+performance-for-range-copy,
10+performance-unnecessary-copy-initialization,

vasild commented at 4:21 am on July 29, 2022:

nit: these were in alphabetical order before, it helps visual search

aureleoules commented at 3:18 pm on August 3, 2022:

fixed thanks

vasild approved

vasild commented at 4:21 am on July 29, 2022: contributor

ACK 46c4ddabb155cf8f0c31519078a3c0beea50725c

tidy: Enable two clang-tidy checks

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

ae7ae36d31

aureleoules force-pushed on Aug 3, 2022

vasild approved

vasild commented at 1:09 pm on August 12, 2022: contributor

ACK ae7ae36d311a869b3bda41d29dc0e47fade77d28

in src/netbase.cpp:482 in ae7ae36d31

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

MarcoFalke commented at 3:10 pm on August 19, 2022:

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

MarcoFalke commented at 3:10 pm on August 19, 2022: member

review ACK ae7ae36d311a869b3bda41d29dc0e47fade77d28

MarcoFalke merged this on Aug 19, 2022

MarcoFalke closed this on Aug 19, 2022

sidhujag referenced this in commit 7755559ac6 on Aug 19, 2022

aureleoules deleted the branch on Aug 25, 2022

PastaPastaPasta referenced this in commit 1995f317a1 on Oct 18, 2022

bitcoin locked this on Aug 25, 2023

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

Conflicts