clang-tidy: Fix performance-*move* warnings in headers #26707

1. 
   hebasto commented at 8:41 pm on December 15, 2022: member

   Split from bitcoin/bitcoin#26705 as was requested in #26705 (comment).

   To test this PR, consider applying a diff as follows:

    0--- a/src/.clang-tidy
    1+++ b/src/.clang-tidy
    2@@ -1,16 +1,7 @@
    3 Checks: '
    4 -*,
    5-bugprone-argument-comment,
    6-bugprone-use-after-move,
    7-misc-unused-using-decls,
    8-modernize-use-default-member-init,
    9-modernize-use-nullptr,
   10-performance-for-range-copy,
   11 performance-move-const-arg,
   12 performance-no-automatic-move,
   13-performance-unnecessary-copy-initialization,
   14-readability-redundant-declaration,
   15-readability-redundant-string-init,
   16 '
   17 WarningsAsErrors: '
   18 bugprone-argument-comment,
   19@@ -28,4 +19,4 @@ readability-redundant-string-init,
   20 CheckOptions:
   21  - key: performance-move-const-arg.CheckTriviallyCopyableMove
   22    value: false
   23-HeaderFilterRegex: './qt'
   24+HeaderFilterRegex: '.'
   

2. 
   DrahtBot commented at 8:41 pm on December 15, 2022: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	fanquake

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   No conflicts as of last run.

3. in src/rpc/util.h:293 in f1f7472c2f outdated

   292@@ -293,11 +293,11 @@ struct RPCResult {
   293         : RPCResult{cond, type, m_key_name, false, description, inner} {}
   

   ---

   ---

   

   maflcko commented at 8:46 pm on December 15, 2022:

   here as well? See also fab680ab9061a2dde0c4882caf002b057c1db8c6 from https://github.com/bitcoin/bitcoin/pull/26706

   ---

   

   hebasto commented at 8:54 pm on December 15, 2022:

   fab680ab9061a2dde0c4882caf002b057c1db8c6 has a wider effect as it adds std::move as well.

   Here, clang-tidy warnings/errors are fixed only. And changes are pretty focused and self-contained.

   I’m not sure how to proceed further. Any suggestions?

   ---

   

   hebasto commented at 11:56 am on January 17, 2023:

   Thanks! Updated.

   ---

   

   maflcko commented at 12:17 pm on January 17, 2023:

   If you rebase directly on fab92a5a5ab45227dbb0aa6d2cf4557b157b17e6, you can avoid changing the commit hash and (potentially) wasting review resources.

   ---

   

   hebasto commented at 12:58 pm on January 17, 2023:

   If you rebase directly on fab92a5, you can avoid changing the commit hash and (potentially) wasting review resources.

   Done.

4. hebasto force-pushed on Jan 17, 2023
5. hebasto renamed this:
   clang-tidy: Fix `performance-move-const-arg` in headers
   clang-tidy: Fix `performance-*move*` warnings in headers
   on Jan 17, 2023
6. 
   hebasto commented at 11:55 am on January 17, 2023: member

   Updated f1f7472c2f0304daa83685b8ab7d0127d03f7ad7 -> 22eb1b449afb93eaa255dc00e28efc203ead7527 (pr26707.01 -> pr26707.02):

   • rebased
   • addressed @MarcoFalke’s comment
7. in src/script/miniscript.h:1381 in 22eb1b449a outdated

   1377@@ -1378,7 +1378,7 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
   1378     assert(constructed.size() == 1);
   1379     assert(constructed[0]->ScriptSize() == script_size);
   1380     if (in.size() > 0) return {};
   1381-    const NodeRef<Key> tl_node = std::move(constructed.front());
   

   ---

   ---

   

   fanquake commented at 11:59 am on January 17, 2023:

   cc @sipa @darosior

   ---

   

   darosior commented at 4:08 pm on January 17, 2023:

   TIL. My understanding is that without this patch tl_node would not be moved a couple lines below when returning from the function. Making it not const fixes this.

   If my understanding is correct, ACK.

   ---

   

   hebasto commented at 4:15 pm on January 17, 2023:

   To be precise, this patch makes a compiler to call a move-constructor instead of a copy-constructor in the return statement.

   ---

   

   maflcko commented at 3:10 pm on January 24, 2023:

   NodeRef is std::shared_ptr<bla>, so this shouldn’t change the compiled binary except for a removed std::atomic add and sub.
8. hebasto force-pushed on Jan 17, 2023
9. 
   maflcko commented at 1:14 pm on January 17, 2023: member

   The diff in #26707#issue-1499057294 does not apply
10. 
    hebasto commented at 1:53 pm on January 17, 2023: member

    The diff in #26707 (comment) does not apply

    Sorry. It should be a reversal one. Fixed.

11. clang-tidy: Fix `performance-move-const-arg` in headers

    See https://clang.llvm.org/extra/clang-tidy/checks/performance/move-const-arg.html

    0a5dc030b9
12. clang-tidy: Fix `performance-no-automatic-move` in headers

    See https://clang.llvm.org/extra/clang-tidy/checks/performance/no-automatic-move.html

    1308b837dc
13. hebasto force-pushed on Jan 18, 2023
14. 
    hebasto commented at 3:50 pm on January 18, 2023: member

    Rebased 70e195ac9ff545fa327e6dc21e0be9bb7b768f81 -> 1308b837dc3499896ca73eafa51ac69b455cef00 (pr26707.03 -> pr26707.04) on top of the merged #26706.
15. fanquake approved
16. 
    fanquake commented at 5:52 pm on January 23, 2023: member

    ACK 1308b837dc3499896ca73eafa51ac69b455cef00
17. maflcko merged this on Jan 24, 2023
18. maflcko closed this on Jan 24, 2023

    ---

19. hebasto deleted the branch on Jan 24, 2023
20. sidhujag referenced this in commit db74fb68cc on Jan 24, 2023
21. bitcoin locked this on Jan 24, 2024

Contributors
hebasto DrahtBot maflcko fanquake darosior