[PATCH] D26203: [ClangTidy - performance-unnecessary-value-param]: Do not issue fix for functions that are referenced outside of callExpr

[Felix Berger via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL286424: [clang-tidy] Do not issue fix for functions that are 
referenced outside of… (authored by flx).

Changed prior to commit:
  https://reviews.llvm.org/D26203?vs=77343=77430#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D26203

Files:
  clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp


Index: 
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- 
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
+++ 
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -253,3 +253,21 @@
   // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the const qualified parameter 'A'
   // CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType& 
A) {
 }
+
+void PositiveOnlyMessageAsReferencedInCompilationUnit(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:75: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void 
PositiveOnlyMessageAsReferencedInCompilationUnit(ExpensiveToCopyType A) {
+}
+
+void ReferenceFunctionOutsideOfCallExpr() {
+  void (*ptr)(ExpensiveToCopyType) = 

+}
+
+void PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:66: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void PositiveMessageAndFixAsFunctionIsCalled(const 
ExpensiveToCopyType& A) {
+}
+
+void ReferenceFunctionByCallingIt() {
+  PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType());
+}
Index: 
clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- 
clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ 
clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -39,6 +39,14 @@
   return true;
 }
 
+bool isReferencedOutsideOfCallExpr(const FunctionDecl ,
+   ASTContext ) {
+  auto Matches = match(declRefExpr(to(functionDecl(equalsNode())),
+   unless(hasAncestor(callExpr(,
+   Context);
+  return !Matches.empty();
+}
+
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
@@ -118,10 +126,14 @@
   "invocation but only used as a const reference; "
   "consider making it a const reference")
   << paramNameOrIndex(Param->getName(), Index);
-  // Do not propose fixes in macros since we cannot place them correctly, or if
-  // function is virtual as it might break overrides.
+  // Do not propose fixes when:
+  // 1. the ParmVarDecl is in a macro, since we cannot place them correctly
+  // 2. the function is virtual as it might break overrides
+  // 3. the function is referenced outside of a call expression within the
+  //compilation unit as the signature change could introduce build errors.
   const auto *Method = llvm::dyn_cast(Function);
-  if (Param->getLocStart().isMacroID() || (Method && Method->isVirtual()))
+  if (Param->getLocStart().isMacroID() || (Method && Method->isVirtual()) ||
+  isReferencedOutsideOfCallExpr(*Function, *Result.Context))
 return;
   for (const auto *FunctionDecl = Function; FunctionDecl != nullptr;
FunctionDecl = FunctionDecl->getPreviousDecl()) {


Index: clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -253,3 +253,21 @@
   // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the const qualified parameter 'A'
   // CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType& A) {
 }
+
+void PositiveOnlyMessageAsReferencedInCompilationUnit(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:75: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void PositiveOnlyMessageAsReferencedInCompilationUnit(ExpensiveToCopyType A) {
+}
+
+void ReferenceFunctionOutsideOfCallExpr() {
+  void (*ptr)(ExpensiveToCopyType) = 
+}
+
+void PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:66: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void PositiveMessageAndFixAsFunctionIsCalled(const ExpensiveToCopyType& A) {
+}
+
+void ReferenceFunctionByCallingIt() {
+  PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType());
+}
Index: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- 

[PATCH] D26203: [ClangTidy - performance-unnecessary-value-param]: Do not issue fix for functions that are referenced outside of callExpr

[Felix Berger via cfe-commits]

flx marked an inline comment as done.
flx added a comment.

Thanks for the review!


https://reviews.llvm.org/D26203



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

[PATCH] D26203: [ClangTidy - performance-unnecessary-value-param]: Do not issue fix for functions that are referenced outside of callExpr

[Felix Berger via cfe-commits]

flx removed rL LLVM as the repository for this revision.
flx updated this revision to Diff 77343.

https://reviews.llvm.org/D26203

Files:
  clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  test/clang-tidy/performance-unnecessary-value-param.cpp


Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -259,4 +259,21 @@
 void PositiveNonConstDeclaration(const ExpensiveToCopyType A) {
   // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the const qualified parameter 'A'
   // CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType& 
A) {
+
+void PositiveOnlyMessageAsReferencedInCompilationUnit(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:75: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void 
PositiveOnlyMessageAsReferencedInCompilationUnit(ExpensiveToCopyType A) {
+}
+
+void ReferenceFunctionOutsideOfCallExpr() {
+  void (*ptr)(ExpensiveToCopyType) = 

+}
+
+void PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:66: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void PositiveMessageAndFixAsFunctionIsCalled(const 
ExpensiveToCopyType& A) {
+}
+
+void ReferenceFunctionByCallingIt() {
+  PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType());
 }
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -39,6 +39,14 @@
   return true;
 }
 
+bool isReferencedOutsideOfCallExpr(const FunctionDecl ,
+   ASTContext ) {
+  auto Matches = match(declRefExpr(to(functionDecl(equalsNode())),
+   unless(hasAncestor(callExpr(,
+   Context);
+  return !Matches.empty();
+}
+
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
@@ -118,10 +126,14 @@
   "invocation but only used as a const reference; "
   "consider making it a const reference")
   << paramNameOrIndex(Param->getName(), Index);
-  // Do not propose fixes in macros since we cannot place them correctly, or if
-  // function is virtual as it might break overrides.
+  // Do not propose fixes when:
+  // 1. the ParmVarDecl is in a macro, since we cannot place them correctly
+  // 2. the function is virtual as it might break overrides
+  // 3. the function is referenced outside of a call expression within the
+  //compilation unit as the signature change could introduce build errors.
   const auto *Method = llvm::dyn_cast(Function);
-  if (Param->getLocStart().isMacroID() || (Method && Method->isVirtual()))
+  if (Param->getLocStart().isMacroID() || (Method && Method->isVirtual()) ||
+  isReferencedOutsideOfCallExpr(*Function, *Result.Context))
 return;
   for (const auto *FunctionDecl = Function; FunctionDecl != nullptr;
FunctionDecl = FunctionDecl->getPreviousDecl()) {


Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -259,4 +259,21 @@
 void PositiveNonConstDeclaration(const ExpensiveToCopyType A) {
   // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the const qualified parameter 'A'
   // CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType& A) {
+
+void PositiveOnlyMessageAsReferencedInCompilationUnit(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:75: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void PositiveOnlyMessageAsReferencedInCompilationUnit(ExpensiveToCopyType A) {
+}
+
+void ReferenceFunctionOutsideOfCallExpr() {
+  void (*ptr)(ExpensiveToCopyType) = 
+}
+
+void PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:66: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void PositiveMessageAndFixAsFunctionIsCalled(const ExpensiveToCopyType& A) {
+}
+
+void ReferenceFunctionByCallingIt() {
+  PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType());
 }
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -39,6 +39,14 @@
   return true;
 }
 
+bool isReferencedOutsideOfCallExpr(const FunctionDecl ,
+   ASTContext ) {
+  auto Matches = match(declRefExpr(to(functionDecl(equalsNode())),
+   unless(hasAncestor(callExpr(,
+  

[PATCH] D26203: [ClangTidy - performance-unnecessary-value-param]: Do not issue fix for functions that are referenced outside of callExpr

[Aaron Ballman via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Aside from a commenting nit, LGTM.




Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:130
+  // Do not propose fixes when:
+  // 1. in macros since we cannot place them correctly
+  // 2. the function is virtual as it might break overrides

Not grammatically correct; should be "1. when the ParmVarDecl is in a macro, 
since..."


Repository:
  rL LLVM

https://reviews.llvm.org/D26203



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

[PATCH] D26203: [ClangTidy - performance-unnecessary-value-param]: Do not issue fix for functions that are referenced outside of callExpr

[Felix Berger via cfe-commits]

flx created this revision.
flx added reviewers: alexfh, sbenza.
flx added a subscriber: cfe-commits.
flx set the repository for this revision to rL LLVM.

Suppress fixes for functions that are referenced within the compilation unit 
outside of a call expression as the signature change could break the code 
referencing the function.

We still issue a warning in this case so that users can decide to manually 
change the function signature.


Repository:
  rL LLVM

https://reviews.llvm.org/D26203

Files:
  clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  test/clang-tidy/performance-unnecessary-value-param.cpp


Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -237,3 +237,21 @@
   ExpensiveToCopyType B;
   B = A;
 }
+
+void PositiveOnlyMessageAsReferencedInCompilationUnit(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:75: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void 
PositiveOnlyMessageAsReferencedInCompilationUnit(ExpensiveToCopyType A) {
+}
+
+void ReferenceFunctionOutsideOfCallExpr() {
+  void (*ptr)(ExpensiveToCopyType) = 

+}
+
+void PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:66: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void PositiveMessageAndFixAsFunctionIsCalled(const 
ExpensiveToCopyType& A) {
+}
+
+void ReferenceFunctionByCallingIt() {
+  PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType());
+}
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -39,6 +39,14 @@
   return true;
 }
 
+bool isReferencedOutsideOfCallExpr(const FunctionDecl ,
+   ASTContext ) {
+  auto Matches = match(declRefExpr(to(functionDecl(equalsNode())),
+   unless(hasAncestor(callExpr(,
+   Context);
+  return !Matches.empty();
+}
+
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
@@ -118,10 +126,14 @@
   "invocation but only used as a const reference; "
   "consider making it a const reference")
   << paramNameOrIndex(Param->getName(), Index);
-  // Do not propose fixes in macros since we cannot place them correctly, or if
-  // function is virtual as it might break overrides.
+  // Do not propose fixes when:
+  // 1. in macros since we cannot place them correctly
+  // 2. the function is virtual as it might break overrides
+  // 3. the function is referenced outside of a call expression within the
+  //compilation unit as the signature change could introduce build errors.
   const auto *Method = llvm::dyn_cast(Function);
-  if (Param->getLocStart().isMacroID() || (Method && Method->isVirtual()))
+  if (Param->getLocStart().isMacroID() || (Method && Method->isVirtual()) ||
+  isReferencedOutsideOfCallExpr(*Function, *Result.Context))
 return;
   for (const auto *FunctionDecl = Function; FunctionDecl != nullptr;
FunctionDecl = FunctionDecl->getPreviousDecl()) {


Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -237,3 +237,21 @@
   ExpensiveToCopyType B;
   B = A;
 }
+
+void PositiveOnlyMessageAsReferencedInCompilationUnit(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:75: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void PositiveOnlyMessageAsReferencedInCompilationUnit(ExpensiveToCopyType A) {
+}
+
+void ReferenceFunctionOutsideOfCallExpr() {
+  void (*ptr)(ExpensiveToCopyType) = 
+}
+
+void PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:66: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void PositiveMessageAndFixAsFunctionIsCalled(const ExpensiveToCopyType& A) {
+}
+
+void ReferenceFunctionByCallingIt() {
+  PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType());
+}
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -39,6 +39,14 @@
   return true;
 }
 
+bool isReferencedOutsideOfCallExpr(const FunctionDecl ,
+   ASTContext ) {
+  auto Matches = match(declRefExpr(to(functionDecl(equalsNode())),
+   unless(hasAncestor(callExpr(,
+   Context);
+  return