[clang-tidy] bugprone-argument-comment: ignore name mismatches for decls from system headers

[george.burgess.iv]

Summary

As of 2a3498e24f97d, we ignore parameter name mismatches for functions in the std:: namespace, since those aren't standardized. It seems reasonable to extend this to all functions which are declared in system headers, since this lint can be a bit noisy otherwise (https://bugs.chromium.org/p/chromium/issues/detail?id=1191507).

I'm on the fence about whether this behavior should be governed by an option. I tended toward no in the current patch, since std:: isn't, but I'm happy to add a CheckSystemHeaderDecls flag or similar if that seems better.

[Eugene.Zelenko]

Why --header-filter command line option or HeaderFilterRegex configuration file option could not solve this problem?

[george.burgess.iv]

Thanks for the comment!

Why --header-filter command line option or HeaderFilterRegex configuration file option could not solve this problem?

AFAICT, --header-filter & HeaderFilterRegex exist to filter diagnostics that occur in matching headers. These diagnostics are influenced by the contents of system headers, but are ultimately emitted inside of source files that have interesting lints, e.g.,

[ub] /l [ 19ms ] ~> cat test.cpp
#include <string.h>
void foo(void *a, const void *b, size_t n) {
  memcpy(/*blah=*/a, b, n);
}
[ub] /l [ 2ms ] ~> clang-tidy --checks='bugprone-argument-comment' test.cpp --header-filter='dont-match-anything' --
1 warning generated.
/l/test.cpp:3:10: warning: argument name 'blah' in comment does not match parameter name '__dest' [bugprone-argument-comment]
  memcpy(/*blah=*/a, b, n);
         ^
/usr/include/string.h:43:39: note: '__dest' declared here
extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
                                      ^
[ub] /l [ 25ms ] ~> clang-tidy --checks='bugprone-argument-comment' test.cpp --config 'HeaderFilterRegex: "dont-match-anything"' --
1 warning generated.
/l/test.cpp:3:10: warning: argument name 'blah' in comment does not match parameter name '__dest' [bugprone-argument-comment]
  memcpy(/*blah=*/a, b, n);
         ^
/usr/include/string.h:43:39: note: '__dest' declared here
extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
                                      ^
[ub] /l [ 22ms ] ~>

So I don't think those help here.

[njames93]

In D99993#2672624, @Eugene.Zelenko wrote:

Why --header-filter command line option or HeaderFilterRegex configuration file option could not solve this problem?

That's for suppressing warnings emitted in header files. The issue this is solving is warnings being emitted at call sites where the callee is in system header files.

[george.burgess.iv]

friendly ping :)

[aaron.ballman]

LGTM! I don't think we need to add an option for this until a user requests one, but I don't feel strongly either way.

[george.burgess.iv]

...This entirely dropped off my radar. Will try to land it now; thanks, all!

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

 #include "../utils/LexerUtils.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 namespace {
-AST_MATCHER(Decl, isFromStdNamespace) {
+AST_MATCHER(Decl, isFromStdNamespaceOrSystemHeader) {
   if (const auto *D = Node.getDeclContext()->getEnclosingNamespaceContext())
-    return D->isStdNamespace();
-  return false;
+    if (D->isStdNamespace())
+      return true;
+  return Node.getASTContext().getSourceManager().isInSystemHeader(
+      Node.getLocation());
 }
 } // namespace
 
 ArgumentCommentCheck::ArgumentCommentCheck(StringRef Name,
                                            ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       StrictMode(Options.getLocalOrGlobal("StrictMode", false)),
       IgnoreSingleArgument(Options.get("IgnoreSingleArgument", false)),
Finder->addMatcher(
                // don't check them against NewCallback's parameter names.
                // FIXME: Make this configurable.
                unless(hasDeclaration(functionDecl(
                    hasAnyName("NewCallback", "NewPermanentCallback")))),
                // Ignore APIs from the standard library, since their names are
                // not specified by the standard, and standard library
                // implementations in practice have to use reserved names to
                // avoid conflicts with same-named macros.
-               unless(hasDeclaration(isFromStdNamespace())))
+               unless(hasDeclaration(isFromStdNamespaceOrSystemHeader())))
           .bind("expr"),
       this);
-  Finder->addMatcher(
-      cxxConstructExpr(unless(hasDeclaration(isFromStdNamespace())))
+  Finder->addMatcher(cxxConstructExpr(unless(hasDeclaration(
+                                          isFromStdNamespaceOrSystemHeader())))
                          .bind("expr"),
                      this);
 }
 
 static std::vector<std::pair<SourceLocation, StringRef>>
 getCommentsInRange(ASTContext *Ctx, CharSourceRange 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());

clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/header-with-decl.h

+void my_header_function(int arg);

clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/system-header-with-decl.h

+#pragma clang system_header
+
+void my_system_header_function(int arg);

clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp

-// RUN: %check_clang_tidy %s bugprone-argument-comment %t
+// RUN: %check_clang_tidy %s bugprone-argument-comment %t -- -- -I %S/Inputs/bugprone-argument-comment
 
 // FIXME: clang-tidy should provide a -verify mode to make writing these checks
 // easier and more accurate.
 
 void ffff(int xxxx, int yyyy);
 
 void f(int x, int y);
 void g() {

 namespace ignore_std_functions {
 void test(int a, int b) {
   std::vector<int> s;
   // verify the check is not fired on std functions.
   s.assign(1, /*value=*/2);
   std::swap(a, /*num=*/b);
 }
 } // namespace ignore_std_functions
+
+namespace regular_header {
+#include "header-with-decl.h"
+void test() {
+  my_header_function(/*not_arg=*/1);
+// CHECK-NOTES: [[@LINE-1]]:22: warning: argument name 'not_arg' in comment does not match parameter name 'arg'
+// CHECK-NOTES: header-with-decl.h:1:29: note: 'arg' declared here
+// CHECK-FIXES: my_header_function(/*not_arg=*/1);
+}
+} // namespace regular_header
+
+namespace system_header {
+#include "system-header-with-decl.h"
+void test() {
+  my_system_header_function(/*not_arg=*/1);
+}
+} // namespace system_header