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

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:221215-tidy-move changing 2 files +3 −3

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:

--- a/src/.clang-tidy
+++ b/src/.clang-tidy
@@ -1,16 +1,7 @@
 Checks: '
 -*,
-bugprone-argument-comment,
-bugprone-use-after-move,
-misc-unused-using-decls,
-modernize-use-default-member-init,
-modernize-use-nullptr,
-performance-for-range-copy,
 performance-move-const-arg,
 performance-no-automatic-move,
-performance-unnecessary-copy-initialization,
-readability-redundant-declaration,
-readability-redundant-string-init,
 '
 WarningsAsErrors: '
 bugprone-argument-comment,
@@ -28,4 +19,4 @@ readability-redundant-string-init,
 CheckOptions:
  - key: performance-move-const-arg.CheckTriviallyCopyableMove
    value: false
-HeaderFilterRegex: './qt'
+HeaderFilterRegex: '.'

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.
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.
hebasto force-pushed on Jan 17, 2023
hebasto renamed this:
~~clang-tidy: Fix `performance-move-const-arg` in headers~~
clang-tidy: Fix `performance-*move*` warnings in headers
on Jan 17, 2023
hebasto commented at 11:55 AM on January 17, 2023: member
Updated f1f7472c2f0304daa83685b8ab7d0127d03f7ad7 -> 22eb1b449afb93eaa255dc00e28efc203ead7527 (pr26707.01 -> pr26707.02):
- rebased
- addressed @MarcoFalke's comment
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.
hebasto force-pushed on Jan 17, 2023
maflcko commented at 1:14 PM on January 17, 2023: member

The diff in #26707#issue-1499057294 does not apply
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.

Type	Reviewers
ACK	fanquake

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

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

0a5dc030b9

clang-tidy: Fix `performance-no-automatic-move` in headers

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

1308b837dc

hebasto force-pushed on Jan 18, 2023
hebasto commented at 3:50 PM on January 18, 2023: member

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

ACK 1308b837dc3499896ca73eafa51ac69b455cef00
maflcko merged this on Jan 24, 2023
maflcko closed this on Jan 24, 2023
hebasto deleted the branch on Jan 24, 2023
sidhujag referenced this in commit db74fb68cc on Jan 24, 2023
bitcoin locked this on Jan 24, 2024

Contributors

Linked (view graph)

#26705 clang-tidy: Fix `modernize-use-default-member-init` in headers and force to check all headers

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

Reviews

Conflicts

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