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

2023-04-02 Thread Logan Gnanapragasam via Phabricator via cfe-commits
gnanabit created this revision.
gnanabit added a reviewer: courbet.
Herald added subscribers: PiotrZSL, carlosgalvezp, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
gnanabit requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

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 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147419

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


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
@@ -33,10 +33,15 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents 
automatic move [performance-no-automatic-move]
 }
 
-Obj PositiveSelfConstValue() {
-  const Obj obj = Make();
-  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 'obj1' prevents 
automatic move [performance-no-automatic-move]
 }
 
 // FIXME: Ideally we would warn here too.
@@ -54,18 +59,32 @@
 // Negatives.
 
 StatusOr Temporary() {
-  return Make();
+  return Make();
 }
 
 StatusOr ConstTemporary() {
   return Make();
 }
 
-StatusOr Nrvo() {
+StatusOr ConvertingMoveConstructor() {
   Obj obj = Make();
   return obj;
 }
 
+Obj ConstNrvo() {
+  const Obj obj = Make();
+  return obj;
+}
+
+Obj NotNrvo(bool b) {
+  Obj obj1;
+  Obj obj2;
+  if (b) {
+return obj1;
+  }
+  return obj2;
+}
+
 StatusOr Ref() {
   Obj &obj = Make();
   return obj;
Index: clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
===
--- clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
@@ -16,6 +16,12 @@
 
 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 @@
   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 @@
cxxConstructExpr(
hasDeclaration(LValueRefCtor),
hasArgument(0, ignoringParenImpCasts(declRefExpr(
-  to(ConstLocalVariable)
+  to(NonNrvoConstLocalVariable)
.bind("ctor_call")),
   this);
 }


Index: clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
@@ -33,10 +33,15 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents automatic move [performance-no-automatic-move]
 }
 
-Obj PositiveSelfConstValue() {
-  const Obj obj = Make();
-  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-mo

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

2023-04-02 Thread Logan Gnanapragasam via Phabricator via cfe-commits
gnanabit updated this revision to Diff 510374.
gnanabit added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147419/new/

https://reviews.llvm.org/D147419

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


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
@@ -33,10 +33,15 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents 
automatic move [performance-no-automatic-move]
 }
 
-Obj PositiveSelfConstValue() {
-  const Obj obj = Make();
-  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 @@
 // Negatives.
 
 StatusOr Temporary() {
-  return Make();
+  return Make();
 }
 
 StatusOr ConstTemporary() {
   return Make();
 }
 
-StatusOr Nrvo() {
+StatusOr ConvertingMoveConstructor() {
   Obj obj = Make();
   return obj;
 }
 
+Obj ConstNrvo() {
+  const Obj obj = Make();
+  return obj;
+}
+
+Obj NotNrvo(bool b) {
+  Obj obj1;
+  Obj obj2;
+  if (b) {
+return obj1;
+  }
+  return obj2;
+}
+
 StatusOr Ref() {
   Obj &obj = Make();
   return obj;
Index: clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
===
--- clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
@@ -16,6 +16,12 @@
 
 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 @@
   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 @@
cxxConstructExpr(
hasDeclaration(LValueRefCtor),
hasArgument(0, ignoringParenImpCasts(declRefExpr(
-  to(ConstLocalVariable)
+  to(NonNrvoConstLocalVariable)
.bind("ctor_call")),
   this);
 }


Index: clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
@@ -33,10 +33,15 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents automatic move [performance-no-automatic-move]
 }
 
-Obj PositiveSelfConstValue() {
-  const Obj obj = Make();
-  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 @@
 // Negatives.
 
 StatusOr Temporary() {
-  return Make();
+  return Make();
 }
 
 StatusOr ConstTemporary() {
   return Make();
 }
 
-StatusOr Nrvo() {
+StatusOr ConvertingMoveConstructor() {
   Obj obj = Make();
   return obj;
 }
 
+Obj ConstNrvo() {
+  const Obj obj = Make();
+  return obj;
+}
+
+Obj NotNrvo(bool b) {
+  Obj obj1;
+  Obj obj2;
+  if 

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

2023-04-03 Thread Logan Gnanapragasam via Phabricator via cfe-commits
gnanabit updated this revision to Diff 510532.
gnanabit added a comment.

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 (gnana...@google.com)" to commit the change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147419/new/

https://reviews.llvm.org/D147419

Files:
  clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
@@ -33,10 +33,15 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents automatic move [performance-no-automatic-move]
 }
 
-Obj PositiveSelfConstValue() {
-  const Obj obj = Make();
-  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 @@
 // Negatives.
 
 StatusOr Temporary() {
-  return Make();
+  return Make();
 }
 
 StatusOr ConstTemporary() {
   return Make();
 }
 
-StatusOr Nrvo() {
+StatusOr ConvertingMoveConstructor() {
   Obj obj = Make();
   return obj;
 }
 
+Obj ConstNrvo() {
+  const Obj obj = Make();
+  return obj;
+}
+
+Obj NotNrvo(bool b) {
+  Obj obj1;
+  Obj obj2;
+  if (b) {
+return obj1;
+  }
+  return obj2;
+}
+
 StatusOr Ref() {
   Obj &obj = Make();
   return obj;
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -271,6 +271,10 @@
   ` when using
   ``DISABLED_`` in the test suite name.
 
+- Fixed a false positive in :doc:`performance-no-automatic-move
+  ` when warning would be
+  emitted for a const local variable to which NRVO is applied.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
===
--- clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
@@ -16,6 +16,12 @@
 
 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 @@
   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 @@
cxxConstructExpr(
hasDeclaration(LValueRefCtor),
hasArgument(0, ignoringParenImpCasts(declRefExpr(
-  to(ConstLocalVariable)
+  to(NonNrvoConstLocalVariable)
.bind("ctor_call")),
   this);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits