[clang-tidy] misc-argument-comment non-strict mode

Summary:
The misc-argument-comment check now ignores leading and trailing underscores and
case. The new `StrictMode` local/global option can be used to switch back to
strict checking.

Add getLocalOrGlobal version for integral types, minor cleanups.

Reviewers: hokein, aaron.ballman

Subscribers: aaron.ballman, Prazek, cfe-commits

Differential Revision: https://reviews.llvm.org/D23135

llvm-svn: 277729

clang-tools-extra/clang-tidy/ClangTidy.h

@@ -73,6 +73,23 @@ public:
     return Result;
   }
 
+  /// \brief Read a named option from the ``Context`` and parse it as an
+  /// integral type ``T``.
+  ///
+  /// Reads the option with the check-local name \p LocalName from local or
+  /// global ``CheckOptions``. Gets local option first. If local is not present,
+  /// falls back to get global option. If global option is not present either,
+  /// returns Default.
+  template <typename T>
+  typename std::enable_if<std::is_integral<T>::value, T>::type
+  getLocalOrGlobal(StringRef LocalName, T Default) const {
+    std::string Value = getLocalOrGlobal(LocalName, "");
+    T Result = Default;
+    if (!Value.empty())
+      StringRef(Value).getAsInteger(10, Result);
+    return Result;
+  }
+
   /// \brief Stores an option with the check-local name \p LocalName with string
   /// value \p Value to \p Options.
   void store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,

clang-tools-extra/clang-tidy/misc/ArgumentCommentCheck.cpp

@@ -22,16 +22,21 @@ namespace misc {
 ArgumentCommentCheck::ArgumentCommentCheck(StringRef Name,
                                            ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
+      StrictMode(Options.getLocalOrGlobal("StrictMode", 0) != 0),
       IdentRE("^(/\\* *)([_A-Za-z][_A-Za-z0-9]*)( *= *\\*/)$") {}
 
+void ArgumentCommentCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "StrictMode", StrictMode);
+}
+
 void ArgumentCommentCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       callExpr(unless(cxxOperatorCallExpr()),
                // NewCallback's arguments relate to the pointed function, don't
                // check them against NewCallback's parameter names.
                // FIXME: Make this configurable.
-               unless(hasDeclaration(functionDecl(anyOf(
+               unless(hasDeclaration(functionDecl(
-                   hasName("NewCallback"), hasName("NewPermanentCallback"))))))
+                   hasAnyName("NewCallback", "NewPermanentCallback")))))
           .bind("expr"),
       this);
   Finder->addMatcher(cxxConstructExpr().bind("expr"), this);
@@ -78,12 +83,13 @@ getCommentsInRange(ASTContext *Ctx, CharSourceRange Range) {
   return Comments;
 }
 
-bool
+bool ArgumentCommentCheck::isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params,
-ArgumentCommentCheck::isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params,
+                                        StringRef ArgName, unsigned ArgIndex) {
-                                   StringRef ArgName, unsigned ArgIndex) {
+  std::string ArgNameLowerStr = ArgName.lower();
-  std::string ArgNameLower = ArgName.lower();
+  StringRef ArgNameLower = ArgNameLowerStr;
+  // The threshold is arbitrary.
   unsigned UpperBound = (ArgName.size() + 2) / 3 + 1;
-  unsigned ThisED = StringRef(ArgNameLower).edit_distance(
+  unsigned ThisED = ArgNameLower.edit_distance(
       Params[ArgIndex]->getIdentifier()->getName().lower(),
       /*AllowReplacements=*/true, UpperBound);
   if (ThisED >= UpperBound)
@@ -100,9 +106,9 @@ ArgumentCommentCheck::isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params,
     // 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(
+    unsigned OtherED = ArgNameLower.edit_distance(II->getName().lower(),
-        II->getName().lower(),
+                                                  /*AllowReplacements=*/true,
-        /*AllowReplacements=*/true, ThisED + Threshold);
+                                                  ThisED + Threshold);
     if (OtherED < ThisED + Threshold)
       return false;
   }
@@ -110,15 +116,24 @@ ArgumentCommentCheck::isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params,
   return true;
 }
 
+static bool sameName(StringRef InComment, StringRef InDecl, bool StrictMode) {
+  if (StrictMode)
+    return InComment == InDecl;
+  InComment = InComment.trim('_');
+  InDecl = InDecl.trim('_');
+  // FIXME: compare_lower only works for ASCII.
+  return InComment.compare_lower(InDecl) == 0;
+}
+
 void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx,
                                          const FunctionDecl *Callee,
                                          SourceLocation ArgBeginLoc,
                                          llvm::ArrayRef<const Expr *> Args) {
   Callee = Callee->getFirstDecl();
-  for (unsigned i = 0,
+  for (unsigned I = 0,
-                e = std::min<unsigned>(Args.size(), Callee->getNumParams());
+                E = std::min<unsigned>(Args.size(), Callee->getNumParams());
-       i != e; ++i) {
+       I != E; ++I) {
-    const ParmVarDecl *PVD = Callee->getParamDecl(i);
+    const ParmVarDecl *PVD = Callee->getParamDecl(I);
     IdentifierInfo *II = PVD->getIdentifier();
     if (!II)
       continue;
@@ -126,28 +141,28 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx,
       // Don't warn on arguments for parameters instantiated from template
       // parameter packs. If we find more arguments than the template definition
       // has, it also means that they correspond to a parameter pack.
-      if (Template->getNumParams() <= i ||
+      if (Template->getNumParams() <= I ||
-          Template->getParamDecl(i)->isParameterPack()) {
+          Template->getParamDecl(I)->isParameterPack()) {
         continue;
       }
     }
 
     CharSourceRange BeforeArgument = CharSourceRange::getCharRange(
-        i == 0 ? ArgBeginLoc : Args[i - 1]->getLocEnd(),
+        I == 0 ? ArgBeginLoc : Args[I - 1]->getLocEnd(),
-        Args[i]->getLocStart());
+        Args[I]->getLocStart());
     BeforeArgument = Lexer::makeFileCharRange(
         BeforeArgument, Ctx->getSourceManager(), Ctx->getLangOpts());
 
     for (auto Comment : getCommentsInRange(Ctx, BeforeArgument)) {
       llvm::SmallVector<StringRef, 2> Matches;
       if (IdentRE.match(Comment.second, &Matches)) {
-        if (Matches[2] != II->getName()) {
+        if (!sameName(Matches[2], II->getName(), StrictMode)) {
           {
             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)) {
+            if (isLikelyTypo(Callee->parameters(), Matches[2], I)) {
               Diag << FixItHint::CreateReplacement(
                           Comment.first,
                           (Matches[1] + II->getName() + Matches[3]).str());
@@ -163,7 +178,7 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx,
 
 void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) {
   const Expr *E = Result.Nodes.getNodeAs<Expr>("expr");
-  if (auto Call = dyn_cast<CallExpr>(E)) {
+  if (const auto *Call = dyn_cast<CallExpr>(E)) {
     const FunctionDecl *Callee = Call->getDirectCallee();
     if (!Callee)
       return;
@@ -171,7 +186,7 @@ void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) {
     checkCallArgs(Result.Context, Callee, Call->getCallee()->getLocEnd(),
                   llvm::makeArrayRef(Call->getArgs(), Call->getNumArgs()));
   } else {
-    auto Construct = cast<CXXConstructExpr>(E);
+    const auto *Construct = cast<CXXConstructExpr>(E);
     checkCallArgs(
         Result.Context, Construct->getConstructor(),
         Construct->getParenOrBraceRange().getBegin(),

clang-tools-extra/clang-tidy/misc/ArgumentCommentCheck.h

@@ -37,8 +37,10 @@ public:
 
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap& Opts) override;
 
 private:
+  const bool StrictMode;
   llvm::Regex IdentRE;
 
   bool isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params, StringRef ArgName,

clang-tools-extra/docs/clang-tidy/checks/misc-argument-comment.rst

@@ -18,3 +18,7 @@ that are placed right before the argument.
   // warning: argument name 'bar' in comment does not match parameter name 'foo'
 
 The check tries to detect typos and suggest automated fixes for them.
+
+Supported options:
+  - `StrictMode` (local or global): when non-zero, the check will ignore leading
+    and trailing underscores and case when comparing parameter names.

clang-tools-extra/test/clang-tidy/misc-argument-comment-strict.cpp

@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy %s misc-argument-comment %t -- \
+// RUN:   -config="{CheckOptions: [{key: StrictMode, value: 1}]}" --
+
+void f(int _with_underscores_);
+void g(int x_);
+void ignores_underscores() {
+  f(/*With_Underscores=*/0);
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: argument name 'With_Underscores' in comment does not match parameter name '_with_underscores_'
+// CHECK-FIXES: f(/*_with_underscores_=*/0);
+  f(/*with_underscores=*/1);
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: argument name 'with_underscores' in comment does not match parameter name '_with_underscores_'
+// CHECK-FIXES: f(/*_with_underscores_=*/1);
+  f(/*_With_Underscores_=*/2);
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: argument name '_With_Underscores_' in comment does not match parameter name '_with_underscores_'
+// CHECK-FIXES: f(/*_with_underscores_=*/2);
+  g(/*X=*/3);
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: argument name 'X' in comment does not match parameter name 'x_'
+// CHECK-FIXES: g(/*x_=*/3);
+}

clang-tools-extra/test/clang-tidy/misc-argument-comment.cpp

@@ -36,8 +36,8 @@ void variadic2(int zzz, Args&&... args);
 
 void templates() {
   variadic(/*xxx=*/0, /*yyy=*/1);
-  variadic2(/*zzZ=*/0, /*xxx=*/1, /*yyy=*/2);
+  variadic2(/*zzU=*/0, /*xxx=*/1, /*yyy=*/2);
-  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: argument name 'zzZ' in comment does not match parameter name 'zzz'
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: argument name 'zzU' in comment does not match parameter name 'zzz'
   // CHECK-FIXES: variadic2(/*zzz=*/0, /*xxx=*/1, /*yyy=*/2);
 }
 
@@ -46,3 +46,8 @@ void qqq(bool aaa);
 void f() { qqq(/*bbb=*/FALSE); }
 // CHECK-MESSAGES: [[@LINE-1]]:16: warning: argument name 'bbb' in comment does not match parameter name 'aaa'
 // CHECK-FIXES: void f() { qqq(/*bbb=*/FALSE); }
+
+void f(bool _with_underscores_);
+void ignores_underscores() {
+  f(/*With_Underscores=*/false);
+}

clang-tools-extra/unittests/clang-tidy/MiscModuleTest.cpp

@@ -23,10 +23,10 @@ TEST(ArgumentCommentCheckTest, ThisEditDistanceAboveThreshold) {
 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); }"));
+                "void f(int xxx, int yyy); void g() { f(/*Zxx=*/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);"));
+                "struct C { C(int xxx, int yyy); }; C c(/*Zxx=*/0, 0);"));
 }
 
 TEST(ArgumentCommentCheckTest, OtherEditDistanceBelowThreshold) {