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

[alexfh]

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.

[Prazek]

LG(TM)

[hokein]

clang-tidy/misc/ArgumentCommentCheck.cpp

29 ↗

(On Diff #66712)

I think we should add a StringMode description in ArgumentCommentCheck's document.

180 ↗

(On Diff #66712)

const auto *

188 ↗

(On Diff #66712)

const auto *

[aaron.ballman]

clang-tidy/misc/ArgumentCommentCheck.cpp

124 ↗

(On Diff #66712)

I think this is going to do the wrong thing for non-ASCII identifiers containing characters for which lowercase comparisons make no sense. I'm okay with that behavior (I don't think it should be a common occurrence), but would like to see a test demonstrating a failure case with a FIXME.

[alexfh]

• Documented the StrictMode option, added a couple of consts.

[alexfh]

clang-tidy/misc/ArgumentCommentCheck.cpp

124 ↗

(On Diff #66712)

compare_lower will only modify ascii characters, all other characters will be compared as is.

As a purely theoretical question: are there characters that have lower-case variants, but it doesn't make sense to compare the lower-case variants? Can you give an example?

[aaron.ballman]

clang-tidy/misc/ArgumentCommentCheck.cpp

124 ↗

(On Diff #66796)

Correct, which means this won't behave properly in some locales with UTF-8 identifiers. Consider Turkish, where İ (U+0130 “Latin Capital Letter I With Dot Above”) is the uppercase form of ı (U+0131 “Latin Small Letter Dotless I”). If the comment contains one version while the identifier contains the other, the comparison will currently fail, while a locale-aware comparison would succeed. You run into similar things with SS vs ß in German as well, where the uppercase form is two characters while the lowercase is only a single character.

[alexfh]

clang-tidy/misc/ArgumentCommentCheck.cpp

124 ↗

(On Diff #66796)

Interesting, though it looks like there's now an official capital ẞ https://en.wikipedia.org/wiki/Capital_%E1%BA%9E (which is not frequently needed anyway, I guess).

At the end of the day, what we get is that the non-strict mode is currently somewhat stricter for non-ascii characters. Similar will happen with all other parts in LLVM that rely on StringRef::compare_lower. I don't think we need a separate test for this _here_, since it's a problem on a completely different level. And I guess the use non-ascii identifiers in C++ will cause much more serious problems than a slightly stricter clang-tidy warning ;]

[aaron.ballman]

clang-tidy/misc/ArgumentCommentCheck.cpp

124 ↗

(On Diff #66796)

We may just have different testing philosophies -- I would advocate for a test because we know of a use case that's broken with this particular use of compare_lower. Not all uses of compare_lower are problematic, after all. However, I'm not going to fight for that test case too hard because this is hopefully an edge case that is low-impact. A FIXME would also suffice.

[alexfh]

• Added a FIXME.

[aaron.ballman]

LGTM!

[alexfh]

clang-tidy/misc/ArgumentCommentCheck.cpp

124 ↗

(On Diff #66796)

I'm reluctant to add a case, since the cost of making it work and maintaining on both linux and windows is higher than the value of it, IMO (it's my take out from writing clang-format's limited support for Unicode).

[alexfh]

hokein: I'm committing the patch; if you have more comments, I'm happy to address them in a follow-up.

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

public:
   get(StringRef LocalName, T Default) const {
     std::string Value = get(LocalName, "");
     T Result = Default;
     if (!Value.empty())
       StringRef(Value).getAsInteger(10, Result);
     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,
              StringRef Value) const;
 
   /// \brief Stores an option with the check-local name \p LocalName with
   /// ``int64_t`` value \p Value to \p Options.
   void store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,

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

 ///
 /// The check tries to detect typos and suggest automated fixes for them.
 class ArgumentCommentCheck : public ClangTidyCheck {
 public:
   ArgumentCommentCheck(StringRef Name, ClangTidyContext *Context);
 
   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,
                     unsigned ArgIndex);
   void checkCallArgs(ASTContext *Ctx, const FunctionDecl *Callee,
                      SourceLocation ArgBeginLoc,
                      llvm::ArrayRef<const Expr *> Args);
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ARGUMENTCOMMENTCHECK_H

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

 
 namespace clang {
 namespace tidy {
 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(
-                   hasName("NewCallback"), hasName("NewPermanentCallback"))))))
+               unless(hasDeclaration(functionDecl(
+                   hasAnyName("NewCallback", "NewPermanentCallback")))))
           .bind("expr"),
       this);
   Finder->addMatcher(cxxConstructExpr().bind("expr"), this);
 }
 
 static std::vector<std::pair<SourceLocation, StringRef>>
 getCommentsInRange(ASTContext *Ctx, CharSourceRange Range) {
   std::vector<std::pair<SourceLocation, StringRef>> Comments;
if (Tok.getKind() == tok::comment) {
           Tok.getLocation(),
           StringRef(Buffer.begin() + CommentLoc.second, Tok.getLength()));
     }
   }
 
   return Comments;
 }
 
-bool
-ArgumentCommentCheck::isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params,
+bool ArgumentCommentCheck::isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params,
                                         StringRef ArgName, unsigned ArgIndex) {
-  std::string ArgNameLower = ArgName.lower();
+  std::string ArgNameLowerStr = 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)
     return false;
 
   for (unsigned I = 0, E = Params.size(); I != E; ++I) {
     if (I == ArgIndex)
       continue;
     IdentifierInfo *II = Params[I]->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);
+    unsigned OtherED = ArgNameLower.edit_distance(II->getName().lower(),
+                                                  /*AllowReplacements=*/true,
+                                                  ThisED + Threshold);
     if (OtherED < ThisED + Threshold)
       return false;
   }
 
   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,
-                e = std::min<unsigned>(Args.size(), Callee->getNumParams());
-       i != e; ++i) {
-    const ParmVarDecl *PVD = Callee->getParamDecl(i);
+  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;
     if (auto Template = Callee->getTemplateInstantiationPattern()) {
       // 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 ||
-          Template->getParamDecl(i)->isParameterPack()) {
+      if (Template->getNumParams() <= I ||
+          Template->getParamDecl(I)->isParameterPack()) {
         continue;
       }
     }
 
     CharSourceRange BeforeArgument = CharSourceRange::getCharRange(
-        i == 0 ? ArgBeginLoc : Args[i - 1]->getLocEnd(),
-        Args[i]->getLocStart());
+        I == 0 ? ArgBeginLoc : Args[I - 1]->getLocEnd(),
+        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());
             }
           }
           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)) {
+  if (const 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);
+    const auto *Construct = cast<CXXConstructExpr>(E);
     checkCallArgs(
         Result.Context, Construct->getConstructor(),
         Construct->getParenOrBraceRange().getBegin(),
         llvm::makeArrayRef(Construct->getArgs(), Construct->getNumArgs()));
   }
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

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

.. code:: c++
 
   void f(bool foo);
 
   ...
   f(/*bar=*/true);
   // 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/trunk/test/clang-tidy/misc-argument-comment-strict.cpp

+// 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/trunk/test/clang-tidy/misc-argument-comment.cpp

 template<typename... Args>
 void variadic(Args&&... args);
 
 template<typename... Args>
 void variadic2(int zzz, Args&&... args);
 
 void templates() {
   variadic(/*xxx=*/0, /*yyy=*/1);
-  variadic2(/*zzZ=*/0, /*xxx=*/1, /*yyy=*/2);
-  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: argument name 'zzZ' in comment does not match parameter name 'zzz'
+  variadic2(/*zzU=*/0, /*xxx=*/1, /*yyy=*/2);
+  // 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);
 }
 
 #define FALSE 0
 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/trunk/unittests/clang-tidy/MiscModuleTest.cpp

 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); }"));
+                "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) {
   EXPECT_NO_CHANGES(ArgumentCommentCheck,
                     "void f(int xxx, int yyy); void g() { f(/*xxy=*/0, 0); }");
 }
 
 } // namespace test
 } // namespace tidy
 } // namespace clang