[clang-tools-extra] r204113 - Add an argument comment checker to clang-tidy.

Peter Collingbourne peter at pcc.me.uk
Mon Mar 17 21:46:45 PDT 2014


Author: pcc
Date: Mon Mar 17 23:46:45 2014
New Revision: 204113

URL: http://llvm.org/viewvc/llvm-project?rev=204113&view=rev
Log:
Add an argument comment checker to clang-tidy.

This checks that parameters named in comments that appear before arguments in
function and constructor calls match the parameter name used in the callee's
declaration. For example:

void f(int x, int y);

void g() {
  f(/*y=*/0, /*z=*/0);
}

contains two violations of the policy, as the names 'x' and 'y' used in the
declaration do not match names 'y' and 'z' used at the call site.

I think there is significant value in being able to check/enforce this policy
as a way of guarding against accidental API misuse and silent breakages
caused by API changes.

Although this pattern appears somewhat frequently in the LLVM codebase,
this policy is not prescribed by the LLVM coding standards at the moment,
so it lives under 'misc'.

Differential Revision: http://llvm-reviews.chandlerc.com/D2914

Added:
    clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.cpp
    clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.h
    clang-tools-extra/trunk/test/clang-tidy/arg-comments.cpp
    clang-tools-extra/trunk/unittests/clang-tidy/MiscModuleTest.cpp
Modified:
    clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
    clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
    clang-tools-extra/trunk/unittests/clang-tidy/CMakeLists.txt
    clang-tools-extra/trunk/unittests/clang-tidy/Makefile

Added: clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.cpp?rev=204113&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.cpp (added)
+++ clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.cpp Mon Mar 17 23:46:45 2014
@@ -0,0 +1,166 @@
+//===--- ArgumentCommentCheck.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 "ArgumentCommentCheck.h"
+#include "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/Token.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void ArgumentCommentCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(callExpr(unless(operatorCallExpr())).bind("expr"), this);
+  Finder->addMatcher(constructExpr().bind("expr"), this);
+}
+
+std::vector<std::pair<SourceLocation, StringRef>>
+ArgumentCommentCheck::getCommentsInRange(ASTContext *Ctx, SourceRange Range) {
+  std::vector<std::pair<SourceLocation, StringRef>> Comments;
+  auto &SM = Ctx->getSourceManager();
+  std::pair<FileID, unsigned> BeginLoc = SM.getDecomposedLoc(Range.getBegin()),
+                              EndLoc = SM.getDecomposedLoc(Range.getEnd());
+
+  if (BeginLoc.first != EndLoc.first)
+    return Comments;
+
+  bool Invalid = false;
+  StringRef Buffer = SM.getBufferData(BeginLoc.first, &Invalid);
+  if (Invalid)
+    return Comments;
+
+  const char *StrData = Buffer.data() + BeginLoc.second;
+
+  Lexer TheLexer(SM.getLocForStartOfFile(BeginLoc.first), Ctx->getLangOpts(),
+                 Buffer.begin(), StrData, Buffer.end());
+  TheLexer.SetCommentRetentionState(true);
+
+  while (true) {
+    Token Tok;
+    if (TheLexer.LexFromRawLexer(Tok))
+      break;
+    if (Tok.getLocation() == Range.getEnd() || Tok.getKind() == tok::eof)
+      break;
+
+    if (Tok.getKind() == tok::comment) {
+      std::pair<FileID, unsigned> CommentLoc =
+          SM.getDecomposedLoc(Tok.getLocation());
+      assert(CommentLoc.first == BeginLoc.first);
+      Comments.emplace_back(
+          Tok.getLocation(),
+          StringRef(Buffer.begin() + CommentLoc.second, Tok.getLength()));
+    }
+  }
+
+  return Comments;
+}
+
+bool
+ArgumentCommentCheck::isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params,
+                                   StringRef ArgName, unsigned ArgIndex) {
+  std::string ArgNameLower = ArgName.lower();
+  unsigned UpperBound = (ArgName.size() + 2) / 3 + 1;
+  unsigned ThisED = StringRef(ArgNameLower).edit_distance(
+      Params[ArgIndex]->getIdentifier()->getName().lower(),
+      /*AllowReplacements=*/true, UpperBound);
+  if (ThisED >= UpperBound)
+    return false;
+
+  for (const auto &Param : Params) {
+    if (&Param - Params.begin() == ArgIndex)
+      continue;
+    IdentifierInfo *II = Param->getIdentifier();
+    if (!II)
+      continue;
+
+    const unsigned Threshold = 2;
+    // Other parameters must be an edit distance at least Threshold more away
+    // from this parameter. This gives us greater confidence that this is a typo
+    // of this parameter and not one with a similar name.
+    unsigned OtherED = StringRef(ArgNameLower).edit_distance(
+        II->getName().lower(),
+        /*AllowReplacements=*/true, ThisED + Threshold);
+    if (OtherED < ThisED + Threshold)
+      return false;
+  }
+
+  return true;
+}
+
+void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx,
+                                         const FunctionDecl *Callee,
+                                         SourceLocation ArgBeginLoc,
+                                         llvm::ArrayRef<const Expr *> Args) {
+  Callee = Callee->getFirstDecl();
+  for (unsigned i = 0,
+                e = std::min<unsigned>(Args.size(), Callee->getNumParams());
+       i != e; ++i) {
+    const ParmVarDecl *PVD = Callee->getParamDecl(i);
+    IdentifierInfo *II = PVD->getIdentifier();
+    if (!II)
+      continue;
+
+    SourceLocation BeginSLoc, EndSLoc = Args[i]->getLocStart();
+    if (i == 0)
+      BeginSLoc = ArgBeginLoc;
+    else
+      BeginSLoc = Args[i - 1]->getLocEnd();
+    if (BeginSLoc.isMacroID() || EndSLoc.isMacroID())
+      continue;
+
+    for (auto Comment :
+         getCommentsInRange(Ctx, SourceRange(BeginSLoc, EndSLoc))) {
+      llvm::SmallVector<StringRef, 2> Matches;
+      if (IdentRE.match(Comment.second, &Matches)) {
+        if (Matches[2] != II->getName()) {
+          {
+            DiagnosticBuilder Diag =
+                diag(Comment.first, "argument name '%0' in comment does not "
+                                    "match parameter name %1")
+                << Matches[2] << II;
+            if (isLikelyTypo(Callee->parameters(), Matches[2], i)) {
+              Diag << FixItHint::CreateReplacement(
+                          Comment.first,
+                          (Matches[1] + II->getName() + Matches[3]).str());
+            }
+          }
+          diag(PVD->getLocation(), "%0 declared here", DiagnosticIDs::Note)
+              << II;
+        }
+      }
+    }
+  }
+}
+
+void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) {
+  const Expr *E = Result.Nodes.getNodeAs<Expr>("expr");
+  if (auto Call = dyn_cast<CallExpr>(E)) {
+    const FunctionDecl *Callee = Call->getDirectCallee();
+    if (!Callee)
+      return;
+
+    checkCallArgs(Result.Context, Callee, Call->getCallee()->getLocEnd(),
+                  llvm::makeArrayRef(Call->getArgs(), Call->getNumArgs()));
+  } else {
+    auto Construct = cast<CXXConstructExpr>(E);
+    checkCallArgs(
+        Result.Context, Construct->getConstructor(),
+        Construct->getParenOrBraceRange().getBegin(),
+        llvm::makeArrayRef(Construct->getArgs(), Construct->getNumArgs()));
+  }
+}
+
+} // namespace tidy
+} // namespace clang

Added: clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.h?rev=204113&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.h (added)
+++ clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.h Mon Mar 17 23:46:45 2014
@@ -0,0 +1,40 @@
+//===--- ArgumentCommentCheck.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_MISC_ARG_COMMENT_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ARG_COMMENT_CHECK_H
+
+#include "../ClangTidy.h"
+#include "llvm/Support/Regex.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief Checks that argument comments match parameter names.
+class ArgumentCommentCheck : public ClangTidyCheck {
+public:
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  llvm::Regex IdentRE{ "^(/\\* *)([_A-Za-z][_A-Za-z0-9]*)( *= *\\*/)$" };
+
+  bool isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params, StringRef ArgName,
+                    unsigned ArgIndex);
+  std::vector<std::pair<SourceLocation, StringRef>>
+  getCommentsInRange(ASTContext *Ctx, SourceRange Range);
+  void checkCallArgs(ASTContext *Ctx, const FunctionDecl *Callee,
+                     SourceLocation ArgBeginLoc,
+                     llvm::ArrayRef<const Expr *> Args);
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ARG_COMMENT_CHECK_H

Modified: clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt?rev=204113&r1=204112&r2=204113&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt Mon Mar 17 23:46:45 2014
@@ -1,6 +1,7 @@
 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
+  ArgumentCommentCheck.cpp
   MiscTidyModule.cpp
 
   LINK_LIBS

Modified: clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp?rev=204113&r1=204112&r2=204113&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp Mon Mar 17 23:46:45 2014
@@ -10,6 +10,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "ArgumentCommentCheck.h"
 
 namespace clang {
 namespace tidy {
@@ -17,6 +18,9 @@ namespace tidy {
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+     CheckFactories.addCheckFactory(
+       "misc-argument-comment",
+       new ClangTidyCheckFactory<ArgumentCommentCheck>());
   }
 };
 

Added: clang-tools-extra/trunk/test/clang-tidy/arg-comments.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/arg-comments.cpp?rev=204113&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/arg-comments.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/arg-comments.cpp Mon Mar 17 23:46:45 2014
@@ -0,0 +1,21 @@
+// RUN: clang-tidy --checks=misc-argument-comment %s -- | FileCheck %s
+
+// FIXME: clang-tidy should provide a -verify mode to make writing these checks
+// easier and more accurate.
+
+// CHECK-NOT: warning
+
+void f(int x, int y);
+
+void ffff(int xxxx, int yyyy);
+
+void g() {
+  // CHECK: [[@LINE+5]]:5: warning: argument name 'y' in comment does not match parameter name 'x'
+  // CHECK: :8:12: note: 'x' declared here
+  // CHECK: [[@LINE+3]]:14: warning: argument name 'z' in comment does not match parameter name 'y'
+  // CHECK: :8:19: note: 'y' declared here
+  // CHECK-NOT: warning
+  f(/*y=*/0, /*z=*/0);
+}
+
+// CHECK-NOT: warning

Modified: clang-tools-extra/trunk/unittests/clang-tidy/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-tidy/CMakeLists.txt?rev=204113&r1=204112&r2=204113&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clang-tidy/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/unittests/clang-tidy/CMakeLists.txt Mon Mar 17 23:46:45 2014
@@ -8,7 +8,8 @@ include_directories(${CLANG_LINT_SOURCE_
 
 add_extra_unittest(ClangTidyTests
   LLVMModuleTest.cpp
-  GoogleModuleTest.cpp)
+  GoogleModuleTest.cpp
+  MiscModuleTest.cpp)
 
 target_link_libraries(ClangTidyTests
   clangAST
@@ -18,5 +19,6 @@ target_link_libraries(ClangTidyTests
   clangTidy
   clangTidyGoogleModule
   clangTidyLLVMModule
+  clangTidyMiscModule
   clangTooling
   )

Modified: clang-tools-extra/trunk/unittests/clang-tidy/Makefile
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-tidy/Makefile?rev=204113&r1=204112&r2=204113&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clang-tidy/Makefile (original)
+++ clang-tools-extra/trunk/unittests/clang-tidy/Makefile Mon Mar 17 23:46:45 2014
@@ -14,7 +14,7 @@ TESTNAME = ClangTidy
 LINK_COMPONENTS := asmparser bitreader support MC MCParser option \
 		 TransformUtils
 USEDLIBS = clangTidy.a clangTidyLLVMModule.a clangTidyGoogleModule.a \
-	   clangTidy.a \
+	   clangTidyMiscModule.a clangTidy.a \
 	   clangStaticAnalyzerFrontend.a clangStaticAnalyzerCheckers.a \
 	   clangStaticAnalyzerCore.a \
 	   clangFormat.a clangTooling.a clangFrontend.a clangSerialization.a \

Added: clang-tools-extra/trunk/unittests/clang-tidy/MiscModuleTest.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-tidy/MiscModuleTest.cpp?rev=204113&view=auto
==============================================================================
--- clang-tools-extra/trunk/unittests/clang-tidy/MiscModuleTest.cpp (added)
+++ clang-tools-extra/trunk/unittests/clang-tidy/MiscModuleTest.cpp Mon Mar 17 23:46:45 2014
@@ -0,0 +1,41 @@
+#include "ClangTidyTest.h"
+#include "google/GoogleTidyModule.h"
+#include "misc/ArgumentCommentCheck.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+#define EXPECT_NO_CHANGES(Check, Code)                                         \
+  EXPECT_EQ(Code, runCheckOnCode<Check>(Code))
+
+TEST(ArgumentCommentCheckTest, CorrectComments) {
+  EXPECT_NO_CHANGES(ArgumentCommentCheck,
+                    "void f(int x, int y); void g() { f(/*x=*/0, /*y=*/0); }");
+  EXPECT_NO_CHANGES(ArgumentCommentCheck,
+                    "struct C { C(int x, int y); }; C c(/*x=*/0, /*y=*/0);");
+}
+
+TEST(ArgumentCommentCheckTest, ThisEditDistanceAboveThreshold) {
+  EXPECT_NO_CHANGES(ArgumentCommentCheck,
+                    "void f(int xxx); void g() { f(/*xyz=*/0); }");
+}
+
+TEST(ArgumentCommentCheckTest, OtherEditDistanceAboveThreshold) {
+  EXPECT_EQ("void f(int xxx, int yyy); void g() { f(/*xxx=*/0, 0); }",
+            runCheckOnCode<ArgumentCommentCheck>(
+                "void f(int xxx, int yyy); void g() { f(/*Xxx=*/0, 0); }"));
+  EXPECT_EQ("struct C { C(int xxx, int yyy); }; C c(/*xxx=*/0, 0);",
+            runCheckOnCode<ArgumentCommentCheck>(
+                "struct C { C(int xxx, int yyy); }; C c(/*Xxx=*/0, 0);"));
+}
+
+TEST(ArgumentCommentCheckTest, OtherEditDistanceBelowThreshold) {
+  EXPECT_NO_CHANGES(ArgumentCommentCheck,
+                    "void f(int xxx, int yyy); void g() { f(/*xxy=*/0, 0); }");
+}
+
+} // namespace test
+} // namespace tidy
+} // namespace clang





More information about the cfe-commits mailing list