[clang-tidy] ignore NRVO const variables in performance-no-automatic-move.

In the following code:

```cc
struct Obj {
  Obj();
  Obj(const Obj &);
  Obj(Obj &&);
  virtual ~Obj();
};

Obj ConstNrvo() {
  const Obj obj;
  return obj;
}
```

performance-no-automatic-move warns about the constness of `obj`. However, NRVO
is applied to `obj`, so the `const` should have no effect on performance.

This change modifies the matcher to exclude NRVO variables.

#clang-tidy

Reviewed By: courbet, PiotrZSL

Differential Revision: https://reviews.llvm.org/D147419

diff --git a/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
index b9c411e..42bf8ff 100644 (file)
--- a/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
@@ -16,6 +16,12 @@ using namespace clang::ast_matchers;
 
 namespace clang::tidy::performance {
 
+namespace {
+
+AST_MATCHER(VarDecl, isNRVOVariable) { return Node.isNRVOVariable(); }
+
+} // namespace
+
 NoAutomaticMoveCheck::NoAutomaticMoveCheck(StringRef Name,
                                            ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
@@ -23,8 +29,9 @@ NoAutomaticMoveCheck::NoAutomaticMoveCheck(StringRef Name,
           utils::options::parseStringList(Options.get("AllowedTypes", ""))) {}
 
 void NoAutomaticMoveCheck::registerMatchers(MatchFinder *Finder) {
-  const auto ConstLocalVariable =
+  const auto NonNrvoConstLocalVariable =
       varDecl(hasLocalStorage(), unless(hasType(lValueReferenceType())),
+              unless(isNRVOVariable()),
               hasType(qualType(
                   isConstQualified(),
                   hasCanonicalType(matchers::isExpensiveToCopy()),
@@ -48,7 +55,7 @@ void NoAutomaticMoveCheck::registerMatchers(MatchFinder *Finder) {
                        cxxConstructExpr(
                            hasDeclaration(LValueRefCtor),
                            hasArgument(0, ignoringParenImpCasts(declRefExpr(
-                                              to(ConstLocalVariable)))))
+                                              to(NonNrvoConstLocalVariable)))))
                            .bind("ctor_call")))))),
       this);
 }

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 8a82e4e..13a06ef 100644 (file)
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -275,6 +275,10 @@ Changes in existing checks
   <clang-tidy/checks/google/readability-avoid-underscore-in-googletest-name>` when using
   ``DISABLED_`` in the test suite name.
 
+- Fixed a false positive in :doc:`performance-no-automatic-move
+  <clang-tidy/checks/performance/no-automatic-move>` when warning would be
+  emitted for a const local variable to which NRVO is applied.
+
 Removed checks
 ^^^^^^^^^^^^^^
 

diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
index 6ca59b6..d365f7d 100644 (file)
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
@@ -33,10 +33,15 @@ NonTemplate PositiveNonTemplateConstValue() {
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents automatic move [performance-no-automatic-move]
 }
 
-Obj PositiveSelfConstValue() {
-  const Obj obj = Make<Obj>();
-  return obj;
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents automatic move [performance-no-automatic-move]
+Obj PositiveCantNrvo(bool b) {
+  const Obj obj1;
+  const Obj obj2;
+  if (b) {
+    return obj1;
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: constness of 'obj1' prevents automatic move [performance-no-automatic-move]
+  }
+  return obj2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj2' prevents automatic move [performance-no-automatic-move]
 }
 
 // FIXME: Ideally we would warn here too.
@@ -54,18 +59,32 @@ StatusOr<Obj> PositiveStatusOrLifetimeExtension() {
 // Negatives.
 
 StatusOr<Obj> Temporary() {
-  return Make<const Obj>();
+  return Make<Obj>();
 }
 
 StatusOr<Obj> ConstTemporary() {
   return Make<const Obj>();
 }
 
-StatusOr<Obj> Nrvo() {
+StatusOr<Obj> ConvertingMoveConstructor() {
   Obj obj = Make<Obj>();
   return obj;
 }
 
+Obj ConstNrvo() {
+  const Obj obj = Make<Obj>();
+  return obj;
+}
+
+Obj NotNrvo(bool b) {
+  Obj obj1;
+  Obj obj2;
+  if (b) {
+    return obj1;
+  }
+  return obj2;
+}
+
 StatusOr<Obj> Ref() {
   Obj &obj = Make<Obj &>();
   return obj;