Page MenuHomePhabricator

Avoid copies in range-for loops and add a warning to detect them
ClosedPublic

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

Details

Summary
See title. Fixing these would otherwise be a continuous process, adding
the warning should keep them from cropping up.

Note that the warning seems to be Clang-only for now.

Some more explanations from the PR comments:

From the pull-request to llvm that added the feature, it warns about:

        for (const Foo &x : Foos), where the range Foos does not return
a copy. This warning will suggest using the non-reference type so the
copy is obvious.

        for (const Foo x : Foos), where the range Foos does return a
reference, but is copied into x. This warning will suggest using the
reference type to prevent a copy from being made.

        for (const Bar &x : Foos), where Bar is constructed from Foo. In
this case, suggest using the non-reference "const Bar" to indicate a
copy is intended to be made, or "const Foo &" to prevent a copy from
being made.

Backport of core PR13480
https://github.com/bitcoin/bitcoin/pull/13480/files

Depends on D3795.

Test Plan

With Clang:

make check
ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR13480
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7034
Build 12115: Bitcoin ABC Teamcity Staging
Build 12114: arc lint + arc unit

Event Timeline

Fabien created this revision.Aug 2 2019, 08:29
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 2 2019, 08:29
deadalnix accepted this revision.Aug 5 2019, 05:11

Does this detect issues from D3795 ? In any case; this is good.

This revision is now accepted and ready to land.Aug 5 2019, 05:11
Fabien added a comment.Aug 5 2019, 06:57

@deadalnix I assumed it (because this PR is a follow-up from the previous one) but I tested to be sure it seems that clang (at least version 6) doesn't catch copies of the map key when looping over a map.