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

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

  20. aureleoules force-pushed on Jul 27, 2022
  21. aureleoules force-pushed on Jul 27, 2022
  22. 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

  23. vasild approved
  24. vasild commented at 4:21 AM on July 29, 2022: contributor

    ACK 46c4ddabb155cf8f0c31519078a3c0beea50725c

  25. tidy: Enable two clang-tidy checks
    Checks enabled: 'performance-for-range-copy' and 'performance-unnecessary-copy-initialization'
    ae7ae36d31
  26. aureleoules force-pushed on Aug 3, 2022
  27. vasild approved
  28. vasild commented at 1:09 PM on August 12, 2022: contributor

    ACK ae7ae36d311a869b3bda41d29dc0e47fade77d28

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

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

    review ACK ae7ae36d311a869b3bda41d29dc0e47fade77d28

  31. MarcoFalke merged this on Aug 19, 2022
  32. MarcoFalke closed this on Aug 19, 2022
  33. sidhujag referenced this in commit 7755559ac6 on Aug 19, 2022
  34. aureleoules deleted the branch on Aug 25, 2022
  35. PastaPastaPasta referenced this in commit 1995f317a1 on Oct 18, 2022
  36. bitcoin locked this on Aug 25, 2023
Contributors
aureleoulesshaavanMarcoFalkefanquakeDrahtBotvasild
Labels
Refactoring
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-04-17 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me