refactor: Use move semantics instead of custom swap functions #26749

hebasto · 2022-12-24T23:38:58Z

This PR makes code more succinct and readable by using move semantics.

DrahtBot · 2022-12-24T23:39:00Z

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	martinus, achow101, TheCharlatan, MarcoFalke
Concept ACK	sipa, aureleoules
Stale ACK	ryanofsky

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.

sipa · 2022-12-31T13:16:01Z

Concept ACK

aureleoules

Concept ACK

maflcko

ACK 3bc1cd3 🍈

Show signature

Signature:

src/validation.cpp

src/validation.h

src/checkqueue.h

src/test/checkqueue_tests.cpp

src/checkqueue.h

hebasto · 2023-01-03T19:33:01Z

Updated 3bc1cd3 -> 94a0f7f (pr26749.01 -> pr26749.02, diff):

addressed @MarcoFalke's comments

maflcko

review ACK 94a0f7f 🗓

Show signature

Signature:

src/bench/checkqueue.cpp

hebasto · 2023-01-15T15:45:59Z

Updated 94a0f7f -> 1e756af (pr26749.02 -> pr26749.03, diff):

addressed @MarcoFalke's comment

maflcko

re-ACK 1e756af 👃

Show signature

Signature:

src/checkqueue.h

src/bench/checkqueue.cpp

maflcko · 2023-01-16T13:45:06Z

est/checkqueue_tests.cpp:165:13: error: 'vChecks' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
            vChecks.resize(std::min(total, (size_t) InsecureRandRange(10)));
            ^
test/checkqueue_tests.cpp:167:21: note: move occurred here
            control.Add(std::move(vChecks));
                    ^

hebasto · 2023-01-16T15:24:16Z

est/checkqueue_tests.cpp:165:13: error: 'vChecks' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
            vChecks.resize(std::min(total, (size_t) InsecureRandRange(10)));
            ^
test/checkqueue_tests.cpp:167:21: note: move occurred here
            control.Add(std::move(vChecks));
                    ^

Fixed. No significant changes in the output of time ./src/test/test_bitcoin -t checkqueue_tests/test_CheckQueue_Correct_* command.

hebasto · 2023-01-16T16:41:19Z

I'm not sure what is wrong with CI-GH integration...

CI: https://cirrus-ci.com/build/5532411732164608

hebasto · 2023-01-18T15:41:53Z

Needs rebase

Rebased 39b38b8 -> 1427188 (pr26749.07 -> pr26749.08).

Co-authored-by: Martin Leitner-Ankerl <[email protected]>

It is safe now, when move semantics is used instead of a custom swap function.

hebasto · 2023-03-21T13:08:30Z

Updated a870025 -> 95ad70a (pr26749.15 -> pr26749.16, diff):

addressed @martinus's comment
addressed @MarcoFalke's comment

martinus · 2023-03-21T14:35:09Z

re-ACK 95ad70a

achow101 · 2023-03-21T21:10:15Z

ACK 95ad70a

sedited · 2023-03-21T21:19:37Z

re-ACK 95ad70a

maflcko · 2023-03-22T09:55:00Z

re-ACK 95ad70a 🚥

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 95ad70ab652ddde7de65f633c36c1378b26a313a 🚥
3Jx5P5EQLnV2phsYvioBMSy53z+Oh8H+i3RnHRzSDyN/ear06Xcd0hZpgZIpjy2ToUbztFvgkSlnwG3PNzfGBQ==

…ructors fae3490 test: Remove unused Check* default constructors (MarcoFalke) Pull request description: They are no longer needed after the removal of `swap`, see bitcoin/bitcoin#26749 (comment) Also, flatten a redundant `if` check. ACKs for top commit: hebasto: ACK fae3490 Tree-SHA512: c0bc0c16b5df0f16fc25e18d2414a2a3c4769da1aa30d53f8d267bc2e97dd79a0296db94c1e49cd1ca89bd42275d8c462f7bf47f03f105dfe867ebea6563454b

…wap fun…

fae3490 test: Remove unused Check* default constructors (MarcoFalke) Pull request description: They are no longer needed after the removal of `swap`, see bitcoin#26749 (comment) Also, flatten a redundant `if` check. ACKs for top commit: hebasto: ACK fae3490 Tree-SHA512: c0bc0c16b5df0f16fc25e18d2414a2a3c4769da1aa30d53f8d267bc2e97dd79a0296db94c1e49cd1ca89bd42275d8c462f7bf47f03f105dfe867ebea6563454b

andrewtoth · 2024-11-29T14:09:22Z

src/checkqueue.h


    //! Add a batch of checks to the queue
-    void Add(std::vector<T>& vChecks) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
+    void Add(std::vector<T>&& vChecks) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)


I don't think this change has any affect, aside from requiring std::move at the callsite.

In the method body we are only moving out of the individual elements, leaving a hollow element in its place. However, the actual vector vChecks is not moved so this would not have any change over using a regular lvalue reference.

This would be a benefit if somewhere in here we took ownership of vChecks, like

if (queue.empty()) { queue = std::move(vChecks); } ``` Actually, even then it wouldn't matter because we can move out of the lvalue reference.

I don't think this change has any affect, aside from requiring std::move at the callsite.

Yes, and the benefits of requiring std::move are:

Self-documenting code, to mark an object as used.

use-after-move static analysis to catch violations, where a depleted object is re-used.

includes a FIXME for rpc_psbt.py that must have happened in the merge of bitcoin/bitcoin#27200

…wap functions

fae3490 test: Remove unused Check* default constructors (MarcoFalke) Pull request description: They are no longer needed after the removal of `swap`, see bitcoin#26749 (comment) Also, flatten a redundant `if` check. ACKs for top commit: hebasto: ACK fae3490 Tree-SHA512: c0bc0c16b5df0f16fc25e18d2414a2a3c4769da1aa30d53f8d267bc2e97dd79a0296db94c1e49cd1ca89bd42275d8c462f7bf47f03f105dfe867ebea6563454b

DrahtBot added the Refactoring label Dec 24, 2022

fanquake requested a review from ryanofsky December 28, 2022 10:15

DrahtBot mentioned this pull request Dec 31, 2022

Drop CCheckQueue::nIdle #26776

Closed

aureleoules approved these changes Jan 2, 2023

View reviewed changes

maflcko reviewed Jan 3, 2023

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

src/checkqueue.h Outdated Show resolved Hide resolved

src/test/checkqueue_tests.cpp Show resolved Hide resolved

src/checkqueue.h Show resolved Hide resolved

hebasto force-pushed the 221224-move branch from 3bc1cd3 to 94a0f7f Compare January 3, 2023 19:28

maflcko approved these changes Jan 6, 2023

View reviewed changes

src/bench/checkqueue.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 221224-move branch from 94a0f7f to 1e756af Compare January 15, 2023 15:43

hebasto changed the title ~~refactor: Use move semanitcs in favour of custom swap functions~~ refactor: Use move semanitcs instead of custom swap functions Jan 15, 2023

maflcko approved these changes Jan 16, 2023

View reviewed changes

src/checkqueue.h Show resolved Hide resolved

src/bench/checkqueue.cpp Outdated Show resolved Hide resolved

maflcko reviewed Jan 16, 2023

View reviewed changes

src/bench/checkqueue.cpp Show resolved Hide resolved

hebasto force-pushed the 221224-move branch from cbf4e59 to 53b0394 Compare January 16, 2023 13:10

hebasto force-pushed the 221224-move branch from 53b0394 to cbd2e1c Compare January 16, 2023 14:00

hebasto marked this pull request as draft January 16, 2023 14:13

hebasto force-pushed the 221224-move branch from cbd2e1c to 39b38b8 Compare January 16, 2023 14:51

hebasto marked this pull request as ready for review January 16, 2023 15:00

hebasto closed this Jan 16, 2023

hebasto reopened this Jan 16, 2023

fanquake changed the title ~~refactor: Use move semanitcs instead of custom swap functions~~ refactor: Use move semantics instead of custom swap functions Jan 16, 2023

DrahtBot mentioned this pull request Jan 17, 2023

bugfix: Make CCheckQueue RAII-styled (attempt 2) #26762

Merged

maflcko added the Needs rebase label Jan 18, 2023

hebasto force-pushed the 221224-move branch from 39b38b8 to 1427188 Compare January 18, 2023 15:08

hebasto and others added 6 commits March 21, 2023 13:04

clang-tidy, test: Fix bugprone-use-after-move in Correct_Queue_range()

9a0b524

refactor: Use move semantics in CCheckQueue::Loop

d8427cc

Co-authored-by: Martin Leitner-Ankerl <[email protected]>

refactor: Drop no longer used CScriptCheck() default constructor

b4bed5c

clang-tidy: Fix modernize-use-default-member-init in CScriptCheck

a87fb6b

refactor: Drop no longer used swap member functions

cea5052

test: Default initialize should_freeze to true

95ad70a

It is safe now, when move semantics is used instead of a custom swap function.

hebasto force-pushed the 221224-move branch from a870025 to 95ad70a Compare March 21, 2023 13:05

DrahtBot requested review from maflcko and sedited March 21, 2023 14:35

DrahtBot removed the request for review from sedited March 21, 2023 21:19

DrahtBot removed the request for review from maflcko March 22, 2023 09:55

fanquake merged commit a709114 into bitcoin:master Mar 22, 2023

hebasto deleted the 221224-move branch March 22, 2023 11:31

maflcko mentioned this pull request Mar 22, 2023

test: Remove unused Check* default constructors #27297

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 23, 2023

Merge bitcoin#26749: refactor: Use move semantics instead of custom s…

bdfb916

…wap fun…

bitcoin locked and limited conversation to collaborators Mar 21, 2024

bitcoin unlocked this conversation Nov 29, 2024

andrewtoth reviewed Nov 29, 2024

View reviewed changes

delta1 added a commit to delta1/elements that referenced this pull request Apr 14, 2025

Merge a709114 into merged_master (Bitcoin PR bitcoin/bitcoin#26749)

e268f3a

includes a FIXME for rpc_psbt.py that must have happened in the merge of bitcoin/bitcoin#27200

DashCoreAutoGuix pushed a commit to DashCoreAutoGuix/dash that referenced this pull request Jul 24, 2025

Merge bitcoin#26749: refactor: Use move semantics instead of custom s…

026869d

…wap functions

DashCoreAutoGuix mentioned this pull request Jul 24, 2025

Merge bitcoin/bitcoin#26749: refactor: Use move semantics instead of custom swap functions DashCoreAutoGuix/dash#349

Closed

bitcoin locked and limited conversation to collaborators Nov 29, 2025

refactor: Use move semantics instead of custom swap functions #26749

refactor: Use move semantics instead of custom swap functions #26749

Uh oh!

Conversation

hebasto commented Dec 24, 2022

Uh oh!

DrahtBot commented Dec 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

sipa commented Dec 31, 2022

Uh oh!

aureleoules left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto commented Jan 3, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Jan 15, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jan 16, 2023

Uh oh!

hebasto commented Jan 16, 2023

Uh oh!

hebasto commented Jan 16, 2023

Uh oh!

hebasto commented Jan 18, 2023

Uh oh!

hebasto commented Mar 21, 2023

Uh oh!

martinus commented Mar 21, 2023

Uh oh!

achow101 commented Mar 21, 2023

Uh oh!

sedited commented Mar 21, 2023

Uh oh!

maflcko commented Mar 22, 2023

Uh oh!

andrewtoth Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 29, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

DrahtBot commented Dec 24, 2022 •

edited

Loading

aureleoules left a comment •

edited

Loading

maflcko left a comment •

edited

Loading

andrewtoth Nov 29, 2024 •

edited

Loading