Author: Martin Boehme Date: 2021-03-05T15:05:09+01:00 New Revision: e67d91faec2183789b220c15444929fb2efa6ecd
URL: https://github.com/llvm/llvm-project/commit/e67d91faec2183789b220c15444929fb2efa6ecd DIFF: https://github.com/llvm/llvm-project/commit/e67d91faec2183789b220c15444929fb2efa6ecd.diff LOG: [clang-tidy] Use-after-move: Ignore moves inside a try_emplace. We have no way to reason about the bool returned by try_emplace, so we simply ignore any std::move()s that happen in a try_emplace argument. A lot of the time in this situation, the code will be checking the bool and doing something else if it turns out the value wasn't moved into the map, and this has been causing false positives so far. I don't currently have any intentions of handling "maybe move" functions more generally. Reviewed By: sammccall Differential Revision: https://reviews.llvm.org/D98034 Added: Modified: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index 028fefa9b99e..136d8f862b95 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -398,7 +398,13 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) { hasArgument(0, declRefExpr().bind("arg")), anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")), hasAncestor(functionDecl().bind("containing-func"))), - unless(inDecltypeOrTemplateArg())) + unless(inDecltypeOrTemplateArg()), + // try_emplace is a common maybe-moving function that returns a + // bool to tell callers whether it moved. Ignore std::move inside + // try_emplace to avoid false positives as we don't track uses of + // the bool. + unless(hasParent(cxxMemberCallExpr( + callee(cxxMethodDecl(hasName("try_emplace"))))))) .bind("call-move"); Finder->addMatcher( diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst index aab7cfd0ccd4..8509292eff99 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst @@ -169,6 +169,10 @@ that a move always takes place: The check will assume that the last line causes a move, even though, in this particular case, it does not. Again, this is intentional. +There is one special case: A call to ``std::move`` inside a ``try_emplace`` call +is conservatively assumed not to move. This is to avoid spurious warnings, as +the check has no way to reason about the ``bool`` returned by ``try_emplace``. + When analyzing the order in which moves, uses and reinitializations happen (see section `Unsequenced moves, uses, and reinitializations`_), the move is assumed to occur in whichever function the result of the ``std::move`` is passed to. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp index 527c79840696..73ca59ccc91b 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp @@ -32,6 +32,31 @@ struct weak_ptr { bool expired() const; }; +template <typename T1, typename T2> +struct pair {}; + +template <typename Key, typename T> +struct map { + struct iterator {}; + + map(); + void clear(); + bool empty(); + template <class... Args> + pair<iterator, bool> try_emplace(const Key &key, Args &&...args); +}; + +template <typename Key, typename T> +struct unordered_map { + struct iterator {}; + + unordered_map(); + void clear(); + bool empty(); + template <class... Args> + pair<iterator, bool> try_emplace(const Key &key, Args &&...args); +}; + #define DECLARE_STANDARD_CONTAINER(name) \ template <typename T> \ struct name { \ @@ -55,11 +80,9 @@ DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(deque); DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(forward_list); DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(list); DECLARE_STANDARD_CONTAINER(set); -DECLARE_STANDARD_CONTAINER(map); DECLARE_STANDARD_CONTAINER(multiset); DECLARE_STANDARD_CONTAINER(multimap); DECLARE_STANDARD_CONTAINER(unordered_set); -DECLARE_STANDARD_CONTAINER(unordered_map); DECLARE_STANDARD_CONTAINER(unordered_multiset); DECLARE_STANDARD_CONTAINER(unordered_multimap); @@ -483,6 +506,22 @@ class IgnoreMemberVariables { } }; +// Ignore moves that happen in a try_emplace. +void ignoreMoveInTryEmplace() { + { + std::map<int, A> amap; + A a; + amap.try_emplace(1, std::move(a)); + a.foo(); + } + { + std::unordered_map<int, A> amap; + A a; + amap.try_emplace(1, std::move(a)); + a.foo(); + } +} + //////////////////////////////////////////////////////////////////////////////// // Tests involving control flow. @@ -776,7 +815,7 @@ void standardContainerClearIsReinit() { container.empty(); } { - std::map<int> container; + std::map<int, int> container; std::move(container); container.clear(); container.empty(); @@ -800,7 +839,7 @@ void standardContainerClearIsReinit() { container.empty(); } { - std::unordered_map<int> container; + std::unordered_map<int, int> container; std::move(container); container.clear(); container.empty(); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
