[clang-tidy] Make the named parameter check only warn on declarations if a definition is visible.

[bkramer]

Summary

This allows us to copy the parameter name from the definition (as a comment)
or insert /*unused*/ in both places.

[alexfh]

clang-tidy/google/NamedParameterCheck.cpp

81 ↗

(On Diff #12151)

I'd make NewName a StringRef which stores the name or "unused", and add " /*" and "*/" when issuing a fixit.

[alexfh]

Looks good with my comment fixed, btw.

[bkramer]

Closed by commit rL214701 (authored by d0k).

clang-tools-extra/trunk/clang-tidy/google/NamedParameterCheck.cpp

void NamedParameterCheck::check(const MatchFinder::MatchResult &Result) {
   const SourceManager &SM = *Result.SourceManager;
   const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("decl");
   SmallVector<std::pair<const FunctionDecl *, unsigned>, 4> UnnamedParams;
 
   // Ignore implicitly generated members.
   if (Function->isImplicit())
     return;
 
+  // Ignore declarations without a definition if we're not dealing with an
+  // overriden method.
+  const FunctionDecl *Definition = nullptr;
+  if (!Function->isDefined(Definition) &&
+      (!isa<CXXMethodDecl>(Function) ||
+       cast<CXXMethodDecl>(Function)->size_overridden_methods() == 0))
+    return;
+
   // TODO: Handle overloads.
   // TODO: We could check that all redeclarations use the same name for
   //       arguments in the same position.
   for (unsigned I = 0, E = Function->getNumParams(); I != E; ++I) {
     const ParmVarDecl *Parm = Function->getParamDecl(I);
     // Look for unnamed parameters.
     if (!Parm->getName().empty())
       continue;
void NamedParameterCheck::check(const MatchFinder::MatchResult &Result) {
 
   // Emit only one warning per function but fixits for all unnamed parameters.
   if (!UnnamedParams.empty()) {
     const ParmVarDecl *FirstParm =
         UnnamedParams.front().first->getParamDecl(UnnamedParams.front().second);
     auto D = diag(FirstParm->getLocation(),
                   "all parameters should be named in a function");
 
+    // Fallback to an unused marker.
+    StringRef NewName = "unused";
+
     for (auto P : UnnamedParams) {
       // If the method is overridden, try to copy the name from the base method
       // into the overrider.
-      const ParmVarDecl *Parm = P.first->getParamDecl(P.second);
       const auto *M = dyn_cast<CXXMethodDecl>(P.first);
       if (M && M->size_overridden_methods() > 0) {
         const ParmVarDecl *OtherParm =
             (*M->begin_overridden_methods())->getParamDecl(P.second);
-        std::string Name = OtherParm->getNameAsString();
-        if (!Name.empty()) {
-          D << FixItHint::CreateInsertion(Parm->getLocation(),
-                                          " /*" + Name + "*/");
-          continue;
-        }
-      }
-
-      // Otherwise just insert an unused marker. Note that getLocation() points
-      // to the place where the name would be, this allows us to also get
-      // complex cases like function pointers right.
-      D << FixItHint::CreateInsertion(Parm->getLocation(), " /*unused*/");
+        StringRef Name = OtherParm->getName();
+        if (!Name.empty())
+          NewName = Name;
+      }
+
+      // If the definition has a named parameter use that name.
+      if (Definition) {
+        const ParmVarDecl *DefParm = Definition->getParamDecl(P.second);
+        StringRef Name = DefParm->getName();
+        if (!Name.empty())
+          NewName = Name;
+      }
+
+      // Now insert the comment. Note that getLocation() points to the place
+      // where the name would be, this allows us to also get complex cases like
+      // function pointers right.
+      const ParmVarDecl *Parm = P.first->getParamDecl(P.second);
+                                      " /*" + NewName.str() + "*/");
     }
   }
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/test/clang-tidy/google-readability-function.cpp

 // RUN: $(dirname %s)/check_clang_tidy_fix.sh %s google-readability-function %t
 // REQUIRES: shell
 
 void Method(char *) { /* */ }
 // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: all parameters should be named in a function
 // CHECK-FIXES: void Method(char * /*unused*/) { /* */ }
-void Method2(char *);
+void Method2(char *) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: all parameters should be named in a function
-// CHECK-FIXES: void Method2(char * /*unused*/);
-void Method3(char *, void *);
+// CHECK-FIXES: void Method2(char * /*unused*/) {}
+void Method3(char *, void *) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: all parameters should be named in a function
-// CHECK-FIXES: void Method3(char * /*unused*/, void * /*unused*/);
-void Method4(char *, int /*unused*/);
+// CHECK-FIXES: void Method3(char * /*unused*/, void * /*unused*/) {}
+void Method4(char *, int /*unused*/) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: all parameters should be named in a function
-// CHECK-FIXES: void Method4(char * /*unused*/, int /*unused*/);
-void operator delete[](void *) throw();
+// CHECK-FIXES: void Method4(char * /*unused*/, int /*unused*/) {}
+void operator delete[](void *) throw() {}
 // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: all parameters should be named in a function
-// CHECK-FIXES: void operator delete[](void * /*unused*/) throw();
-int Method5(int);
+// CHECK-FIXES: void operator delete[](void * /*unused*/) throw() {}
+int Method5(int) { return 0; }
 // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: all parameters should be named in a function
-// CHECK-FIXES: int Method5(int /*unused*/);
+// CHECK-FIXES: int Method5(int /*unused*/) { return 0; }
 void Method6(void (*)(void *)) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: all parameters should be named in a function
 // CHECK-FIXES: void Method6(void (* /*unused*/)(void *)) {}
-template <typename T> void Method7(T);
+template <typename T> void Method7(T) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: all parameters should be named in a function
-// CHECK-FIXES: template <typename T> void Method7(T /*unused*/);
+// CHECK-FIXES: template <typename T> void Method7(T /*unused*/) {}
 
 // Don't warn in macros.
-#define M void MethodM(int);
+#define M void MethodM(int) {}
 M
 
-void operator delete(void *x) throw();
+void operator delete(void *x) throw() {}
 void Method7(char * /*x*/) {}
-void Method8(char *x);
+void Method8(char *x) {}
 typedef void (*TypeM)(int x);
 void operator delete[](void *x) throw();
 void operator delete[](void * /*x*/) throw();
 
 struct X {
-  X operator++(int);
+  X operator++(int) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: all parameters should be named in a function
-// CHECK-FIXES: X operator++(int /*unused*/);
-  X operator--(int /*unused*/);
+// CHECK-FIXES: X operator++(int /*unused*/) {}
+  X operator--(int /*unused*/) {}
 
   const int &i;
 };
 
 void (*Func1)(void *);
 void Func2(void (*func)(void *)) {}
-template <void Func(void *)> void Func3();
+template <void Func(void *)> void Func3() {}
 
 template <typename T>
 struct Y {
-  void foo(T);
+  void foo(T) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: all parameters should be named in a function
-// CHECK-FIXES: void foo(T /*unused*/);
+// CHECK-FIXES: void foo(T /*unused*/) {}
 };
 
 Y<int> y;
 Y<float> z;
 
 struct Base {
   virtual void foo(bool notThisOne);
   virtual void foo(int argname);
 };
 
 struct Derived : public Base {
   void foo(int);
 // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: all parameters should be named in a function
 // CHECK-FIXES: void foo(int /*argname*/);
 };
+
+void FDef(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: all parameters should be named in a function
+// CHECK-FIXES: void FDef(int /*n*/);
+void FDef(int n) {};
+
+void FNoDef(int);