Replace std::list with std::vector where appropriate #29827
+421
−400
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@juli27 Would you mind taking a look at this PR? |
juli27
reviewed
Sep 12, 2025
src/framework/global/containers.h
Outdated
Comment on lines
45
to
47
| inline bool contains_if(const std::vector<T>& vec, Predicate pred) | ||
| { | ||
| return std::find_if(vec.cbegin(), vec.cend(), pred) != vec.cend(); |
There was a problem hiding this comment.
nit: forwarding reference & forward to avoid copy. (std::function can heap allocate)
Suggested change
| inline bool contains_if(const std::vector<T>& vec, Predicate pred) | |
| { | |
| return std::find_if(vec.cbegin(), vec.cend(), pred) != vec.cend(); | |
| inline bool contains_if(const std::vector<T>& vec, Predicate&& pred) | |
| { | |
| return std::find_if(vec.cbegin(), vec.cend(), std::forward<Predicate>(pred)) != vec.cend(); |
src/engraving/dom/undo.cpp
Outdated
| @@ -650,7 +653,7 @@ bool UndoMacro::empty() const | |||
|
|
|||
| void UndoMacro::append(UndoMacro&& other) | |||
| { | |||
| appendChildren(&other); | |||
| appendChildren(std::forward<UndoMacro>(other)); | |||
There was a problem hiding this comment.
This is pretty unusual and unsafe usage of rvalue references, because we state here that other is moved into appendChildren but then continue to use it further down this function. It works only because we don't actually move the entire UndoMacro in appendChildren but only the child commands vector. Let's get rid of the move iterator in appendChildren, which only copies the raw pointers anyway
There was a problem hiding this comment.
Thanks! I pushed some code review fixes in a separate commit.
But with a std::vector range represented by a start and end iterator. Instead of popping from a list, we're now shrinking the vector range, until it's empty. Involves consistent usage of std::remove and std::remove_if, also with reverse iterators. std::remove[_if] are of course in theory a little bit slower than std::list::remove[_if], because they need to `std::move` some values around. But according to the literature, that cost is easily outweighed by the benefits of std::vector, like cache-friendliness.
There were a few `remove` calls, but not significant enough to warrant `std::list`, given that the lists here should be relatively small.
Co-Authored-By: Julian Bühler <[email protected]>
cbjeukendrup
force-pushed
the
std_list
branch
from
390437e to
6855563
Compare
juli27
approved these changes
Sep 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves: #12219 (almost completely)