tidy: enable bugprone-use-after-move #25733

fanquake commented at 9:33 AM on July 29, 2022: member

Would have caught #25640.

Currently // NOLINTs around:

test/util_tests.cpp:2513:34: error: 't2' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
    BOOST_CHECK(v2[0].origin == &t2);
                                 ^
test/util_tests.cpp:2511:15: note: move occurred here
    auto v2 = Vector(std::move(t2));
              ^
test/util_tests.cpp:2519:34: error: 't2' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
    BOOST_CHECK(v3[1].origin == &t2);
                                 ^
test/util_tests.cpp:2516:15: note: move occurred here
    auto v3 = Vector(t1, std::move(t2));
              ^
test/util_tests.cpp:2527:34: error: 't3' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
    BOOST_CHECK(v4[2].origin == &t3);
                                 ^
test/util_tests.cpp:2523:15: note: move occurred here
    auto v4 = Vector(std::move(v3[0]), v3[1], std::move(t3));

See: https://releases.llvm.org/14.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone-use-after-move.html

fanquake added the label Tests on Jul 29, 2022

jonatack commented at 4:39 PM on July 29, 2022: contributor

Concept ACK, looks like it is finding cases in test/util_tests.cpp as well.

test/util_tests.cpp:2497:34: error: 't2' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
    BOOST_CHECK(v2[0].origin == &t2);
                                 ^
test/util_tests.cpp:2495:15: note: move occurred here
    auto v2 = Vector(std::move(t2));
              ^
test/util_tests.cpp:2503:34: error: 't2' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
    BOOST_CHECK(v3[1].origin == &t2);
                                 ^
test/util_tests.cpp:2500:15: note: move occurred here
    auto v3 = Vector(t1, std::move(t2));
              ^
test/util_tests.cpp:2511:34: error: 't3' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
    BOOST_CHECK(v4[2].origin == &t3);
                                 ^
test/util_tests.cpp:2507:15: note: move occurred here
    auto v4 = Vector(std::move(v3[0]), v3[1], std::move(t3));

fanquake force-pushed on Jul 30, 2022

fanquake force-pushed on Aug 1, 2022

DrahtBot commented at 5:07 AM on August 5, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25780 (tidy: Enable more clang-tidy bugprone checks by aureleoules)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ryanofsky commented at 7:15 PM on August 18, 2022: contributor

Concept ACK, since this does seem like it could catch unintentional mistakes, but I have a little skepticism because it can be perfectly ok to use an object after moving. For example, vector's move constructor guarantees the moved from object will be empty (https://en.cppreference.com/w/cpp/container/vector/vector).

Currently some things are failing in CI as jonatack pointed out:

https://github.com/bitcoin/bitcoin/blob/e25b0c6838f375c810bee292ed49e13c9b5acc69/src/test/util_tests.cpp#L2495-L2497

But maybe we could work around these with a custom move function (util::SafelyMoveFrom?) that does the same as std::move but avoids triggering the error. Or, I wonder if something like std::move(*&t2) would trick the linter

fanquake commented at 8:25 AM on August 19, 2022: member

Or, I wonder if something like std::move(*&t2) would trick the linter

Looks like it does (locally). I've pushed this up for now, but will defer to you as to what change / how you'd rather handle this in the util tests.

fanquake force-pushed on Aug 19, 2022

in src/test/util_tests.cpp:2513 in 696434d7a0 outdated

2507 | @@ -2508,19 +2508,19 @@ BOOST_AUTO_TEST_CASE(test_tracked_vector)
2508 |      BOOST_CHECK(v1[0].origin == &t1);
2509 |      BOOST_CHECK_EQUAL(v1[0].copies, 1);
2510 |  
2511 | -    auto v2 = Vector(std::move(t2));
2512 | +    auto v2 = Vector(std::move(*&t2));
2513 |      BOOST_CHECK_EQUAL(v2.size(), 1U);
2514 |      BOOST_CHECK(v2[0].origin == &t2);

ajtowns commented at 7:20 AM on August 29, 2022:

Shouldn't you just write // NOLINT(*-use-after-move) to disable the check for these lines (which also documents that you're deliberately doing weird things) ? https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics

ryanofsky commented at 5:24 PM on August 29, 2022:

Makes sense, I agree // NOLINT(*-use-after-move) looks better than what I suggested (SafelyMoveFrom(t2) or std::move(*&t2)). I didn't know what syntax to disable linter was, but I assumed it would be worse than that.

I think ideally the compiler or linter would have a usable-after-moved directive that could be applied to types like std::vector where using after move is safe and well defined, so individual moves wouldn't have to be annotated. But either way works

fanquake commented at 2:21 PM on August 30, 2022:

Switched to // NOLINT.

ryanofsky approved

ryanofsky commented at 5:30 PM on August 29, 2022: contributor

Code review ACK 696434d7a03a4e028484142aa2d80e4f0a63c60c. It's nice to enable tidy checks to catch unintended uses after moves. Workaround for intended uses seems ok, and NOLINT alternate suggestion would be ok too.

test: work around bugprone-use-after-move warnings in util tests

```bash
test/util_tests.cpp:2513:34: error: 't2' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
    BOOST_CHECK(v2[0].origin == &t2);
                                 ^
test/util_tests.cpp:2511:15: note: move occurred here
    auto v2 = Vector(std::move(t2));
              ^
test/util_tests.cpp:2519:34: error: 't2' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
    BOOST_CHECK(v3[1].origin == &t2);
                                 ^
test/util_tests.cpp:2516:15: note: move occurred here
    auto v3 = Vector(t1, std::move(t2));
              ^
test/util_tests.cpp:2527:34: error: 't3' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
    BOOST_CHECK(v4[2].origin == &t3);
                                 ^
test/util_tests.cpp:2523:15: note: move occurred here
    auto v4 = Vector(std::move(v3[0]), v3[1], std::move(t3));
```

94f2235f85

tidy: enable bugprone-use-after-move

Will error with:
```bash
coins.cpp:102:22: error: 'coin' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
           (uint32_t)coin.nHeight,
                     ^
coins.cpp:96:21: note: move occurred here
    it->second.coin = std::move(coin);
```

until #25663 is merged.

See:
https://releases.llvm.org/14.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone-use-after-move.html

f345dc3960

fanquake force-pushed on Aug 30, 2022

ryanofsky approved

ryanofsky commented at 5:58 PM on August 30, 2022: contributor

Code review ACK f345dc3960c2cf4d69ebbcc011e4e836205f0361. Only change since last review is switching to NOLINT directives

MarcoFalke merged this on Aug 30, 2022

MarcoFalke closed this on Aug 30, 2022

sidhujag referenced this in commit 1ed11c09ae on Aug 31, 2022

fanquake deleted the branch on Aug 31, 2022

PastaPastaPasta referenced this in commit 48b85e3744 on Oct 18, 2022

bitcoin locked this on Aug 31, 2023