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

pull aureleoules wants to merge 2 commits into bitcoin:master from aureleoules:2022-07-cleanup changing 28 files +43 −39
  1. 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.

  2. aureleoules renamed this:
    2022 07 cleanup
    refactor: Cleanup variables
    on Jul 26, 2022
  3. aureleoules renamed this:
    refactor: Cleanup variables
    refactor: Cleanup unused variables
    on Jul 26, 2022
  4. DrahtBot added the label Refactoring on Jul 26, 2022
  5. 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.

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

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

  8. aureleoules force-pushed on Jul 26, 2022
  9. aureleoules force-pushed on Jul 26, 2022
  10. aureleoules force-pushed on Jul 26, 2022
  11. aureleoules force-pushed on Jul 26, 2022
  12. aureleoules renamed this:
    refactor: Cleanup unused variables
    refactor: Cleanup unused variables and enable two clang-tidy checks
    on Jul 26, 2022
  13. 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.

  14. DrahtBot commented at 5:31 PM on July 26, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  15. DrahtBot cross-referenced this on Jul 26, 2022 from issue refactor: Avoid UniValue copies by MarcoFalke
  16. DrahtBot cross-referenced this on Jul 26, 2022 from issue wallet: Load database records in a particular order by achow101
  17. DrahtBot cross-referenced this on Jul 26, 2022 from issue wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101
  18. 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).

  19. refactor: Make const refs vars where applicable
    This avoids initializing variables with the copy-constructor of a
non-trivially copyable type.
    081b0e53e3
  20. aureleoules force-pushed on Jul 27, 2022
  21. 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
  22. 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.

  23. fanquake cross-referenced this on Jul 27, 2022 from issue refactor: make member functions const when applicable by aureleoules
  24. aureleoules force-pushed on Jul 27, 2022
  25. aureleoules force-pushed on Jul 27, 2022
  26. 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

  27. vasild approved
  28. vasild commented at 4:21 AM on July 29, 2022: contributor

    ACK 46c4ddabb155cf8f0c31519078a3c0beea50725c

  29. tidy: Enable two clang-tidy checks
    Checks enabled: 'performance-for-range-copy' and 'performance-unnecessary-copy-initialization'
    ae7ae36d31
  30. aureleoules force-pushed on Aug 3, 2022
  31. DrahtBot cross-referenced this on Aug 4, 2022 from issue tidy: add modernize-use-using by fanquake
  32. DrahtBot cross-referenced this on Aug 11, 2022 from issue rpc: add getxpub by Sjors
  33. vasild approved
  34. vasild commented at 1:09 PM on August 12, 2022: contributor

    ACK ae7ae36d311a869b3bda41d29dc0e47fade77d28

  35. DrahtBot cross-referenced this on Aug 19, 2022 from issue Fix issues when calling std::move(const&) by MarcoFalke
  36. 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

  37. MarcoFalke commented at 3:10 PM on August 19, 2022: member

    review ACK ae7ae36d311a869b3bda41d29dc0e47fade77d28

  38. MarcoFalke merged this on Aug 19, 2022
  39. MarcoFalke closed this on Aug 19, 2022
  40. sidhujag referenced this in commit 7755559ac6 on Aug 19, 2022
  41. aureleoules deleted the branch on Aug 25, 2022
  42. PastaPastaPasta referenced this in commit 1995f317a1 on Oct 18, 2022
  43. aureleoules cross-referenced this on Dec 1, 2022 from issue test: remove unused vars in `feature_block` by brunoerg
  44. bitcoin locked this on Aug 25, 2023
Contributors
aureleoulesshaavanMarcoFalkefanquakeDrahtBotvasild
Labels
Refactoring
Linked (view graph)
#25673 refactor: make member functions const when applicable#26507 test: remove unused vars in `feature_block`
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-05-20 06:53 UTC