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

[gnanabit]

Summary

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

[gnanabit]

Fix typo in CHECK-MESSAGES (should warn about obj2, not obj1).

[courbet]

Thanks for the patch.

[PiotrZSL]

Current change is correct.
This check could be improved in future.
LGTM.

[PiotrZSL]

Add release notes entry !.

[gnanabit]

Add release notes entry.

Thanks for the reviews! Happy to reword the release notes if they are unclear.

@courbet or @PiotrZSL, I don't have commit access. Can you land this patch for
me? Please use "Logan Gnanapragasam ([e-mail])" to commit the change.

clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp

 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 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),
       AllowedTypes(
           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()),
                   unless(hasDeclaration(namedDecl(
                       matchers::matchesAnyListedName(AllowedTypes)))))))
           .bind("vardecl");
 
   // A matcher for a `DstT::DstT(const Src&)` where DstT also has a
   // `DstT::DstT(Src&&)`.
   const auto LValueRefCtor = cxxConstructorDecl(
       hasParameter(0,
                    hasType(lValueReferenceType(pointee(type().bind("SrcT"))))),
       ofClass(cxxRecordDecl(hasMethod(cxxConstructorDecl(
           hasParameter(0, hasType(rValueReferenceType(
                               pointee(type(equalsBoundNode("SrcT")))))))))));
 
   Finder->addMatcher(
       traverse(TK_AsIs,
                returnStmt(hasReturnValue(
                    ignoringElidableConstructorCall(ignoringParenImpCasts(
                        cxxConstructExpr(
                            hasDeclaration(LValueRefCtor),
                            hasArgument(0, ignoringParenImpCasts(declRefExpr(
-                                              to(ConstLocalVariable)))))
+                                              to(NonNrvoConstLocalVariable)))))
                            .bind("ctor_call")))))),
       this);
 }
 
 void NoAutomaticMoveCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Var = Result.Nodes.getNodeAs<VarDecl>("vardecl");
   const auto *CtorCall = Result.Nodes.getNodeAs<Expr>("ctor_call");
   diag(CtorCall->getExprLoc(), "constness of '%0' prevents automatic move")

clang-tools-extra/docs/ReleaseNotes.rst

- Fixed an issue in :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>` when specifying an empty
   string for ``Prefix`` or ``Suffix`` options could result in the style not
   being used.
 
 - Fixed an issue in :doc:`google-readability-avoid-underscore-in-googletest-name
   <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
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp

 }
 
 NonTemplate PositiveNonTemplateConstValue() {
   const Obj obj = Make<Obj>();
   return obj;
   // 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.
 NonTemplate PositiveNonTemplateLifetimeExtension() {
   const Obj &obj = Make<Obj>();
   return obj;
 }
 
 // FIXME: Ideally we would warn here too.
 StatusOr<Obj> PositiveStatusOrLifetimeExtension() {
   const Obj &obj = Make<Obj>();
   return obj;
 }
 
 // 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;
 }
 
 StatusOr<Obj> ConstRef() {
   const Obj &obj = Make<Obj &>();
   return obj;