Page MenuHomePhabricator

scripted-diff: Avoid temporary copies when looping over std::map
ClosedPublic

Authored by Fabien on Aug 2 2019, 08:14.

Details

Summary
The ::value_type of the std::map/std::multimap/std::unordered_map
containers is std::pair<const Key, T>. Dropping the const results in an
unnecessary copy, for example in C++11 range-based loops.
For this I started with a more general scripted diff, then narrowed it
down based on the inspection showing that all actual
map/multimap/unordered_map variables used in loops start with m or have
map in the name.

-BEGIN VERIFY SCRIPT-
sed -i -E 's/for \(([^<]*)std::pair<([^c])(.+) : m/for
(\1std::pair<const \2\3 : m/' src/*.cpp src/**/*.cpp
sed -i -E 's/for \(([^<]*)std::pair<([^c])(.+) : (.*)map/for
(\1std::pair<const \2\3 : \4map/' src/*.cpp src/**/*.cpp
-END VERIFY SCRIPT-

Backport of core PR13241
https://github.com/bitcoin/bitcoin/pull/13241/files

Note to reviewers:
There is a couple of changes not backported from the PR, due to missing
dependencies regarding the wallet label API. A follow-up diff will
backport PR13480 and introduce a clang warning to detect copies in
loops, so I think this is safe to leave it for now.

Test Plan
make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR13241
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7033
Build 12113: Bitcoin ABC Buildbot (legacy)
Build 12112: arc lint + arc unit

Event Timeline

This kind of stuff should definitively have a linter. clang-tidy seems like good candidate.

This revision is now accepted and ready to land.Aug 3 2019, 07:21

@deadalnix D3796 introduces a new warning that catch these cases (only with clang).