[clang-tidy] Add a checker that flags unnamed parameters.

[bkramer]

Summary

We still allow the escape hatch foo(int /*x*/) and also suggest this
in a fixit. This is more powerful than the corresponding cpplint.py check
it also flags functions with multiple arguments as naming all arguments is
recommended by the google style guide.

[djasper]

clang-tidy/google/NamedParameterCheck.cpp

63 ↗

(On Diff #11437)

IMO, most functions with unnamed parameters will be derived from some base method. Otherwise, why would they have an unnamed parameter (yes, there are cases, but not that many)?

To this end:

• Add tests for methods overriding methods with named parameters.
• I think the common pattern for the unnamed parameter comment is /*<parameter name>*/ Could we extract the parameter name from the overridden function.

[alexfh]

clang-tidy/google/NamedParameterCheck.cpp

31 ↗

(On Diff #11437)

Why don't you check for isImplicit()?

36 ↗

(On Diff #11437)

Alternatively this could be done in the swapped arguments check. Both places seem good for this.

50 ↗

(On Diff #11437)

I'm curious where does Parm->getLocation() point in case where there is a parameter name (to the identifier?) and where there is not?

clang-tidy/google/NamedParameterCheck.h

19 ↗

(On Diff #11437)

nit: Do we need an empty line to separate the \brief from the rest of the class comment?

test/clang-tidy/google-readability-function.cpp

1 ↗

(On Diff #11437)

Please add tests with multiple instantiations of the same template. I suspect, currently the check will suggest multiple replacements.

[bkramer]

clang-tidy/google/NamedParameterCheck.cpp

50 ↗

(On Diff #11437)

It always points to the name or the place where the name *would* be. This behavior is a bit strange sometimes but makes things like getting the name in function pointers right very easy,

test/clang-tidy/google-readability-function.cpp

1 ↗

(On Diff #11437)

I checked that and it doesn't (do warnings get deduplicated somewhere)? I'll add another test.

[bkramer]

• Added a testcase with multiple template instantiations.
• If a method has an unnamed argument and is overriding a method from a base cla

ss try to copy the argument name from the base class first.

[alexfh]

test/clang-tidy/google-readability-function.cpp

69 ↗

(On Diff #11439)

What does the check do in this case?

struct Base1 { virtual void f(int i) {} };
struct Base2 { virtual void f(bool b) {} };
template <typename T, typename Y>
struct Derived : public T {
  void f(Y) {}
};
int main() {
  Derived<Base1, int>();
  Derived<Base2, bool>();
}

1 ↗

(On Diff #11437)

Warnings get deduplicated by location and message text, and in your test case deduplication worked correctly. But it's not a good idea to rely on the deduplication in general.

[bkramer]

test/clang-tidy/google-readability-function.cpp

69 ↗

(On Diff #11439)

I think it works because we see the template definition first. But I'll add the bulky template instance matcher, safe is better.

5:13: warning: all parameters should be named in a function [google-readability-function]
    void f(Y) {}
            ^
             /*unused*/

[bkramer]

Added a check so we don't emit warnings inside of template instantiations.

[alexfh]

Looks good!

clang-tidy/google/NamedParameterCheck.cpp

73 ↗

(On Diff #11443)

Add a comma after "overridden"?

[bkramer]

Closed by commit rL213075 (authored by d0k).

clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyGoogleModule
   AvoidCStyleCastsCheck.cpp
   ExplicitConstructorCheck.cpp
   ExplicitMakePairCheck.cpp
   GoogleTidyModule.cpp
+  NamedParameterCheck.cpp
   OverloadedUnaryAndCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

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

 //===--- GoogleTidyModule.cpp - clang-tidy --------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidCStyleCastsCheck.h"
 #include "ExplicitConstructorCheck.h"
 #include "ExplicitMakePairCheck.h"
+#include "NamedParameterCheck.h"
 #include "OverloadedUnaryAndCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 
 class GoogleModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.addCheckFactory(
         "google-build-explicit-make-pair",
         new ClangTidyCheckFactory<build::ExplicitMakePairCheck>());
     CheckFactories.addCheckFactory(
         "google-explicit-constructor",
         new ClangTidyCheckFactory<ExplicitConstructorCheck>());
     CheckFactories.addCheckFactory(
         "google-runtime-operator",
         new ClangTidyCheckFactory<runtime::OverloadedUnaryAndCheck>());
     CheckFactories.addCheckFactory(
         "google-readability-casting",
         new ClangTidyCheckFactory<readability::AvoidCStyleCastsCheck>());
+    CheckFactories.addCheckFactory(
+        "google-readability-function",
+        new ClangTidyCheckFactory<readability::NamedParameterCheck>());
   }
 };
 
 // Register the GoogleTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<GoogleModule> X("google-module",
                                                     "Adds Google lint checks.");
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the GoogleModule.
 volatile int GoogleModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

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

+//===--- NamedParameterCheck.h - clang-tidy ---------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_NAMED_PARAMETER_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_NAMED_PARAMETER_CHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// \brief Find functions with unnamed arguments.
+///
+/// Corresponding cpplint.py check name: 'readability/function'.
+class NamedParameterCheck : public ClangTidyCheck {
+public:
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_NAMED_PARAMETER_CHECK_H

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

+//===--- NamedParameterCheck.cpp - clang-tidy -------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "NamedParameterCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/AST/ASTContext.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void NamedParameterCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(
+      functionDecl(
+          unless(hasAncestor(decl(
+              anyOf(recordDecl(ast_matchers::isTemplateInstantiation()),
+                    functionDecl(ast_matchers::isTemplateInstantiation()))))))
+          .bind("decl"),
+      this);
+}
+
+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;
+
+  // 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;
+
+    // Sanity check the source locations.
+    if (!Parm->getLocation().isValid() || Parm->getLocation().isMacroID() ||
+        !SM.isWrittenInSameFile(Parm->getLocStart(), Parm->getLocation()))
+      continue;
+
+    // Look for comments. We explicitly want to allow idioms like
+    // void foo(int /*unused*/)
+    const char *Begin = SM.getCharacterData(Parm->getLocStart());
+    const char *End = SM.getCharacterData(Parm->getLocation());
+    StringRef Data(Begin, End - Begin);
+    if (Data.find("/*") != StringRef::npos)
+      continue;
+
+    UnnamedParams.push_back(std::make_pair(Function, I));
+  }
+
+  // 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");
+
+    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*/");
+    }
+  }
+}
+
+} // 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 *);
+// 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-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-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-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-MESSAGES: :[[@LINE-1]]:16: warning: all parameters should be named in a function
+// CHECK-FIXES: int Method5(int /*unused*/);
+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);
+// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: all parameters should be named in a function
+// CHECK-FIXES: template <typename T> void Method7(T /*unused*/);
+
+// Don't warn in macros.
+#define M void MethodM(int);
+M
+
+void operator delete(void *x) throw();
+void Method7(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);
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: all parameters should be named in a function
+// 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 <typename T>
+struct Y {
+  void foo(T);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: all parameters should be named in a function
+// 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*/);
+};