Re: [PATCH] D20010: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified

2016-05-30 Thread Felix Berger via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL271239: [clang-tidy] UnnecessaryCopyInitialization - Extend 
to trigger on non-const… (authored by flx).

Changed prior to commit:
  http://reviews.llvm.org/D20010?vs=58181=59010#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D20010

Files:
  
clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h
  
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-copy-initialization.cpp

Index: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h
===
--- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h
+++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h
@@ -33,7 +33,8 @@
 
 private:
   void handleCopyFromMethodReturn(const VarDecl , const Stmt ,
-  bool IssueFix, ASTContext );
+  bool IssueFix, const VarDecl *ObjectArg,
+  ASTContext );
   void handleCopyFromLocalVar(const VarDecl , const VarDecl ,
   const Stmt , bool IssueFix,
   ASTContext );
Index: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
===
--- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -38,14 +38,15 @@
 unless(allOf(pointerType(), unless(pointerType(pointee(
 qualType(isConstQualified(;
 
-  // Match method call expressions where the this argument is a const
-  // type or const reference. This returned const reference is highly likely to
-  // outlive the local const reference of the variable being declared.
-  // The assumption is that the const reference being returned either points
-  // to a global static variable or to a member of the called object.
-  auto ConstRefReturningMethodCallOfConstParam = cxxMemberCallExpr(
+  // Match method call expressions where the `this` argument is only used as
+  // const, this will be checked in `check()` part. This returned const
+  // reference is highly likely to outlive the local const reference of the
+  // variable being declared. The assumption is that the const reference being
+  // returned either points to a global static variable or to a member of the
+  // called object.
+  auto ConstRefReturningMethodCall = cxxMemberCallExpr(
   callee(cxxMethodDecl(returns(ConstReference))),
-  on(declRefExpr(to(varDecl(hasType(qualType(ConstOrConstReference)));
+  on(declRefExpr(to(varDecl().bind("objectArg");
   auto ConstRefReturningFunctionCall =
   callExpr(callee(functionDecl(returns(ConstReference))),
unless(callee(cxxMethodDecl(;
@@ -68,7 +69,7 @@
 
   Finder->addMatcher(
   localVarCopiedFrom(anyOf(ConstRefReturningFunctionCall,
-   ConstRefReturningMethodCallOfConstParam)),
+   ConstRefReturningMethodCall)),
   this);
 
   Finder->addMatcher(localVarCopiedFrom(declRefExpr(
@@ -80,6 +81,7 @@
 const MatchFinder::MatchResult ) {
   const auto *NewVar = Result.Nodes.getNodeAs("newVarDecl");
   const auto *OldVar = Result.Nodes.getNodeAs("oldVarDecl");
+  const auto *ObjectArg = Result.Nodes.getNodeAs("objectArg");
   const auto *BlockStmt = Result.Nodes.getNodeAs("blockStmt");
   const auto *CtorCall = Result.Nodes.getNodeAs("ctorCall");
   // Do not propose fixes if the DeclStmt has multiple VarDecls or in macros
@@ -96,19 +98,23 @@
   return;
 
   if (OldVar == nullptr) {
-handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, *Result.Context);
+handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, ObjectArg,
+   *Result.Context);
   } else {
 handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, IssueFix,
*Result.Context);
   }
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 const VarDecl , const Stmt , bool IssueFix,
-ASTContext ) {
+const VarDecl *ObjectArg, ASTContext ) {
   bool IsConstQualified = Var.getType().isConstQualified();
   if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
 return;
+  if (ObjectArg != nullptr &&
+  !isOnlyUsedAsConst(*ObjectArg, BlockStmt, Context))
+return;
 
   auto Diagnostic =
   diag(Var.getLocation(),
Index: clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-copy-initialization.cpp
===
---

Re: [PATCH] D20010: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified

2016-05-27 Thread Alexander Kornienko via cfe-commits 
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

LG


http://reviews.llvm.org/D20010



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20010: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified

2016-05-26 Thread Felix Berger via cfe-commits 
flx added a comment.

Friendly ping.


http://reviews.llvm.org/D20010



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20010: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified

2016-05-24 Thread Matt Kulukundis via cfe-commits 
fowles added a comment.

LGTM, but I will wait for people who know this stuff better to sign off


http://reviews.llvm.org/D20010



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20010: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified

2016-05-23 Thread Felix Berger via cfe-commits 
flx added a comment.

In http://reviews.llvm.org/D20010#427510, @alexfh wrote:

> How many more (in relative numbers) results does this check generate now?


147% more :)


http://reviews.llvm.org/D20010



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20010: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified

2016-05-23 Thread Felix Berger via cfe-commits 
flx removed rL LLVM as the repository for this revision.
flx updated this revision to Diff 58181.
flx marked an inline comment as done.

http://reviews.llvm.org/D20010

Files:
  clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tidy/performance/UnnecessaryCopyInitialization.h
  test/clang-tidy/performance-unnecessary-copy-initialization.cpp

Index: test/clang-tidy/performance-unnecessary-copy-initialization.cpp
===
--- test/clang-tidy/performance-unnecessary-copy-initialization.cpp
+++ test/clang-tidy/performance-unnecessary-copy-initialization.cpp
@@ -36,81 +36,81 @@
   // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned' is copy-constructed from a const reference; consider making it a const reference [performance-unnecessary-copy-initialization]
   // CHECK-FIXES: const auto& AutoAssigned = ExpensiveTypeReference();
   const auto AutoCopyConstructed(ExpensiveTypeReference());
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
   // CHECK-FIXES: const auto& AutoCopyConstructed(ExpensiveTypeReference());
   const ExpensiveToCopyType VarAssigned = ExpensiveTypeReference();
-  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
   // CHECK-FIXES:   const ExpensiveToCopyType& VarAssigned = ExpensiveTypeReference();
   const ExpensiveToCopyType VarCopyConstructed(ExpensiveTypeReference());
-  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
   // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(ExpensiveTypeReference());
 }
 
 void PositiveMethodCallConstReferenceParam(const ExpensiveToCopyType ) {
   const auto AutoAssigned = Obj.reference();
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
   // CHECK-FIXES: const auto& AutoAssigned = Obj.reference();
   const auto AutoCopyConstructed(Obj.reference());
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
   // CHECK-FIXES: const auto& AutoCopyConstructed(Obj.reference());
   const ExpensiveToCopyType VarAssigned = Obj.reference();
-  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
   // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = Obj.reference();
   const ExpensiveToCopyType VarCopyConstructed(Obj.reference());
-  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
   // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(Obj.reference());
 }
 
 void PositiveMethodCallConstParam(const ExpensiveToCopyType Obj) {
   const auto AutoAssigned = Obj.reference();
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
   // CHECK-FIXES: const auto& AutoAssigned = Obj.reference();
   const auto AutoCopyConstructed(Obj.reference());
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
   // CHECK-FIXES: const auto& AutoCopyConstructed(Obj.reference());
   const ExpensiveToCopyType VarAssigned = Obj.reference();
-  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
   // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = Obj.reference();
   const ExpensiveToCopyType VarCopyConstructed(Obj.reference());
-  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
   // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(Obj.reference());
 }
 
 void PositiveMethodCallConstPointerParam(const ExpensiveToCopyType *const Obj) {
   const auto AutoAssigned = Obj->reference();
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
   // CHECK-FIXES: const auto& AutoAssigned = Obj->reference();
   const auto AutoCopyConstructed(Obj->reference());
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14:

Re: [PATCH] D20010: [clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified

2016-05-11 Thread Alexander Kornienko via cfe-commits 
alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

How many more (in relative numbers) results does this check generate now?



Comment at: clang-tidy/performance/UnnecessaryCopyInitialization.cpp:45
@@ -44,7 +44,3 @@
 
-  // Match method call expressions where the this argument is a const
-  // type or const reference. This returned const reference is highly likely to
-  // outlive the local const reference of the variable being declared.
-  // The assumption is that the const reference being returned either points
-  // to a global static variable or to a member of the called object.
-  auto ConstRefReturningMethodCallOfConstParam = cxxMemberCallExpr(
+  // Match method call expressions where the 'this' argument is only used as
+  // const, this will be checked in check() part. This returned const reference

Please enclose inline code snippets in backquotes (`this`, `check()`, etc.).


Comment at: test/clang-tidy/performance-unnecessary-copy-initialization.cpp:133
@@ -132,3 +132,3 @@
   auto AutoCopyConstructed(ExpensiveTypeReference());
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable
   // CHECK-FIXES: const auto& AutoCopyConstructed(ExpensiveTypeReference());

Please include dynamic parts of the message to the CHECK lines (in this case - 
the variable name).


Repository:
  rL LLVM

http://reviews.llvm.org/D20010



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits