diff options
author | AMS21 <AMS21.github@gmail.com> | 2024-03-04 19:56:15 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-03-04 19:56:15 +0100 |
commit | b32845cb94a81e6fd8b01a8631e3d276c9fc9e35 (patch) | |
tree | af1959f2d169dc4929281b8e9e15f7bfdb5e4fa3 /clang-tools-extra | |
parent | 081882eb9ca63ae9582399beb8b76c1d135d7062 (diff) | |
download | llvm-b32845cb94a81e6fd8b01a8631e3d276c9fc9e35.zip llvm-b32845cb94a81e6fd8b01a8631e3d276c9fc9e35.tar.gz llvm-b32845cb94a81e6fd8b01a8631e3d276c9fc9e35.tar.bz2 |
[clang-tidy] Let `bugprone-use-after-move` also handle calls to `std::forward` (#82673)
Add support for std::forward.
Fixes #82023
Diffstat (limited to 'clang-tools-extra')
4 files changed, 95 insertions, 15 deletions
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index c5b6b54..b91ad0f 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -330,7 +330,8 @@ void UseAfterMoveFinder::getReinits( traverse(TK_AsIs, DeclRefMatcher), unless(parmVarDecl(hasType( references(qualType(isConstQualified())))))), - unless(callee(functionDecl(hasName("::std::move"))))))) + unless(callee(functionDecl( + hasAnyName("::std::move", "::std::forward"))))))) .bind("reinit"); Stmts->clear(); @@ -359,24 +360,46 @@ void UseAfterMoveFinder::getReinits( } } +enum class MoveType { + Move, // std::move + Forward, // std::forward +}; + +static MoveType determineMoveType(const FunctionDecl *FuncDecl) { + if (FuncDecl->getName() == "move") + return MoveType::Move; + if (FuncDecl->getName() == "forward") + return MoveType::Forward; + + llvm_unreachable("Invalid move type"); +} + static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg, const UseAfterMove &Use, ClangTidyCheck *Check, - ASTContext *Context) { - SourceLocation UseLoc = Use.DeclRef->getExprLoc(); - SourceLocation MoveLoc = MovingCall->getExprLoc(); + ASTContext *Context, MoveType Type) { + const SourceLocation UseLoc = Use.DeclRef->getExprLoc(); + const SourceLocation MoveLoc = MovingCall->getExprLoc(); + + const bool IsMove = (Type == MoveType::Move); - Check->diag(UseLoc, "'%0' used after it was moved") - << MoveArg->getDecl()->getName(); - Check->diag(MoveLoc, "move occurred here", DiagnosticIDs::Note); + Check->diag(UseLoc, "'%0' used after it was %select{forwarded|moved}1") + << MoveArg->getDecl()->getName() << IsMove; + Check->diag(MoveLoc, "%select{forward|move}0 occurred here", + DiagnosticIDs::Note) + << IsMove; if (Use.EvaluationOrderUndefined) { - Check->diag(UseLoc, - "the use and move are unsequenced, i.e. there is no guarantee " - "about the order in which they are evaluated", - DiagnosticIDs::Note); + Check->diag( + UseLoc, + "the use and %select{forward|move}0 are unsequenced, i.e. " + "there is no guarantee about the order in which they are evaluated", + DiagnosticIDs::Note) + << IsMove; } else if (UseLoc < MoveLoc || Use.DeclRef == MoveArg) { Check->diag(UseLoc, - "the use happens in a later loop iteration than the move", - DiagnosticIDs::Note); + "the use happens in a later loop iteration than the " + "%select{forward|move}0", + DiagnosticIDs::Note) + << IsMove; } } @@ -388,7 +411,9 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) { auto TryEmplaceMatcher = cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace")))); auto CallMoveMatcher = - callExpr(argumentCountIs(1), callee(functionDecl(hasName("::std::move"))), + callExpr(argumentCountIs(1), + callee(functionDecl(hasAnyName("::std::move", "::std::forward")) + .bind("move-decl")), hasArgument(0, declRefExpr().bind("arg")), unless(inDecltypeOrTemplateArg()), unless(hasParent(TryEmplaceMatcher)), expr().bind("call-move"), @@ -436,6 +461,7 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) { const auto *CallMove = Result.Nodes.getNodeAs<CallExpr>("call-move"); const auto *MovingCall = Result.Nodes.getNodeAs<Expr>("moving-call"); const auto *Arg = Result.Nodes.getNodeAs<DeclRefExpr>("arg"); + const auto *MoveDecl = Result.Nodes.getNodeAs<FunctionDecl>("move-decl"); if (!MovingCall || !MovingCall->getExprLoc().isValid()) MovingCall = CallMove; @@ -470,7 +496,8 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) { UseAfterMoveFinder Finder(Result.Context); UseAfterMove Use; if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use)) - emitDiagnostic(MovingCall, Arg, Use, this, Result.Context); + emitDiagnostic(MovingCall, Arg, Use, this, Result.Context, + determineMoveType(MoveDecl)); } } diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 70ae23b..1e19497 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -144,6 +144,10 @@ Changes in existing checks <clang-tidy/checks/bugprone/unused-return-value>` check by updating the parameter `CheckedFunctions` to support regexp. +- Improved :doc:`bugprone-use-after-move + <clang-tidy/checks/bugprone/use-after-move>` check to also handle + calls to ``std::forward``. + - Improved :doc:`cppcoreguidelines-missing-std-forward <clang-tidy/checks/cppcoreguidelines/missing-std-forward>` check by no longer giving false positives for deleted functions. 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 8509292..08bb537 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 @@ -177,6 +177,18 @@ 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. +The check also handles perfect-forwarding with ``std::forward`` so the +following code will also trigger a use-after-move warning. + +.. code-block:: c++ + + void consume(int); + + void f(int&& i) { + consume(std::forward<int>(i)); + consume(std::forward<int>(i)); // use-after-move + } + Use --- 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 00b1da1..7d9f634 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 @@ -111,6 +111,18 @@ constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) noexcept { return static_cast<typename remove_reference<_Tp>::type &&>(__t); } +template <class _Tp> +constexpr _Tp&& +forward(typename std::remove_reference<_Tp>::type& __t) noexcept { + return static_cast<_Tp&&>(__t); +} + +template <class _Tp> +constexpr _Tp&& +forward(typename std::remove_reference<_Tp>::type&& __t) noexcept { + return static_cast<_Tp&&>(__t); +} + } // namespace std class A { @@ -1525,3 +1537,28 @@ public: private: std::string val_; }; + +namespace issue82023 +{ + +struct S { + S(); + S(S&&); +}; + +void consume(S s); + +template <typename T> +void forward(T&& t) { + consume(std::forward<T>(t)); + consume(std::forward<T>(t)); + // CHECK-NOTES: [[@LINE-1]]:27: warning: 't' used after it was forwarded + // CHECK-NOTES: [[@LINE-3]]:11: note: forward occurred here +} + +void create() { + S s; + forward(std::move(s)); +} + +} // namespace issue82023 |