[clang-tools-extra] [clang-tidy] Fix some false positive in bugprone-move-forwarding-reference (PR #191435)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-tools-extra

Author: serge-sans-paille

<details>
<summary>Changes</summary>

In the following case:

template &lt;typename T, typename U&gt;
void shocase(U&amp;&amp; SomeU) {
  [SomeU] () { T SomeT(std::move(SomeU)); };
}

We use to flag the move as a forward, while the lambda captures SomeU by copy, 
which makes the move valid.

---
Full diff: https://github.com/llvm/llvm-project/pull/191435.diff


3 Files Affected:

- (modified) 
clang-tools-extra/clang-tidy/bugprone/MoveForwardingReferenceCheck.cpp (+11-1) 
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) 
- (modified) 
clang-tools-extra/test/clang-tidy/checkers/bugprone/move-forwarding-reference.cpp
 (+22) 


``````````diff
diff --git 
a/clang-tools-extra/clang-tidy/bugprone/MoveForwardingReferenceCheck.cpp 
b/clang-tools-extra/clang-tidy/bugprone/MoveForwardingReferenceCheck.cpp
index e182df75b1d9a..7cc3d33da1a78 100644
--- a/clang-tools-extra/clang-tidy/bugprone/MoveForwardingReferenceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/MoveForwardingReferenceCheck.cpp
@@ -13,6 +13,14 @@ using namespace clang::ast_matchers;
 
 namespace clang::tidy::bugprone {
 
+namespace {
+
+AST_MATCHER(DeclRefExpr, refersToEnclosingVariableOrCapture) {
+  return Node.refersToEnclosingVariableOrCapture();
+}
+
+}
+
 static void replaceMoveWithForward(const UnresolvedLookupExpr *Callee,
                                    const ParmVarDecl *ParmVar,
                                    const TemplateTypeParmDecl *TypeParmDecl,
@@ -86,7 +94,9 @@ void 
MoveForwardingReferenceCheck::registerMatchers(MatchFinder *Finder) {
                           .bind("lookup")),
                argumentCountIs(1),
                hasArgument(0, ignoringParenImpCasts(declRefExpr(
-                                  to(ForwardingReferenceParmMatcher)))))
+                                  to(ForwardingReferenceParmMatcher),
+                                  // FIXME: allow capture by reference
+                                  
unless(refersToEnclosingVariableOrCapture())))))
           .bind("call-move"),
       this);
 }
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst 
b/clang-tools-extra/docs/ReleaseNotes.rst
index caf0275035064..8a2dbfca8384b 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -256,6 +256,10 @@ Changes in existing checks
   <clang-tidy/checks/bugprone/macro-parentheses>` check by printing the macro
   definition in the warning message if the macro is defined on command line.
 
+- Improved :doc:`bugprone-move-forwarding-reference
+  <clang-tidy/checks/bugprone/move-forwarding-reference>` check by removing 
some
+  false positive in the context of moved lambda captures.
+
 - Improved :doc:`bugprone-pointer-arithmetic-on-polymorphic-object
   <clang-tidy/checks/bugprone/pointer-arithmetic-on-polymorphic-object>` check
   by fixing a false positive when ``operator[]`` is used in a dependent 
context.
diff --git 
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-forwarding-reference.cpp
 
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-forwarding-reference.cpp
index c9f40668f449a..f100d6f1d3f3d 100644
--- 
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-forwarding-reference.cpp
+++ 
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/move-forwarding-reference.cpp
@@ -111,3 +111,25 @@ template <typename T> void f12() {
   // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference passed to
   // CHECK-FIXES: [] (auto&& x) { T SomeT(std::forward<decltype(x)>(x)); };
 }
+
+// Ignore the case of captured variables where an implicit copy already
+// happened. Explicit capture version.
+template <typename T, typename U> void f13(U&& SomeU) {
+  [SomeU] () { T SomeT(std::move(SomeU)); };
+}
+
+// Ignore the case of captured variables where an implicit copy already
+// happened. Implicit capture version.
+template <typename T, typename U> void f14(U&& SomeU) {
+  [=] () { T SomeT(std::move(SomeU)); };
+}
+
+#if 0
+// FIXME: we currently ignore that case.
+//
+// Handle the case of captured variables where no copy already
+// happened.
+template <typename T, typename U> void f15(U&& SomeU) {
+  [&SomeU] () { T SomeT(std::move(SomeU)); };
+}
+#endif

``````````

</details>


https://github.com/llvm/llvm-project/pull/191435
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits