tidy: enable bugprone-use-after-move

[fanquake]

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

[jonatack]

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));

[DrahtBot]

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]

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:

bitcoin/src/test/util_tests.cpp

Lines 2495 to 2497 in e25b0c6

	auto v2 = Vector(std::move(t2));
	BOOST_CHECK_EQUAL(v2.size(), 1U);
	BOOST_CHECK(v2[0].origin == &t2);

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]

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.

[ryanofsky]

Code review ACK 696434d. 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.

[ryanofsky]

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