[clang-tidy] Add options to bugprone-argument-comment to add missing argument comments to literals

[MyDeveloperDay]

Summary

bugprone-argument-comment only supports identifying those comments which do not match the function parameter name

This revision add 3 options to adding missing argument comments to literals (granularity on type is added to control verbosity of fixit)

CheckOptions:
  - key:             bugprone-argument-comment.CommentBoolLiterals
    value:           '1'
  - key:             bugprone-argument-comment.CommentFloatLiterals
    value:           '1'
  - key:             bugprone-argument-comment.CommentIntegerLiterals
    value:           '1'
  - key:             bugprone-argument-comment.CommentStringLiterals
    value:           '1'
  - key:             bugprone-argument-comment.CommentCharacterLiterals
    value:           '1'
  - key:             bugprone-argument-comment.CommentUserDefinedLiterals
    value:           '1'
  - key:             bugprone-argument-comment.CommentNullPtrs
    value:           '1'

After applying these options, literal arguments will be preceded with /*ParameterName=*/

[MyDeveloperDay]

An example of this checker run over clang-tidy/modernize is given here D57675: [clang-tidy] DO NOT SUBMIT.. example diff of applying bugprone-argument-comments with AddCommentsToXXXLiterals options

[MyDeveloperDay]

docs/ReleaseNotes.rst

94

note to self, I will reorganize this alphabetically, on next revision

[MyDeveloperDay]

Fix release note alphabetic order
Add examples of Fixit into the documentation

[Eugene.Zelenko]

docs/ReleaseNotes.rst

82

Please move changes in existing checks after list of new checks.

[aaron.ballman]

Can you drop the file mode changes that are in this review?

clang-tidy/bugprone/ArgumentCommentCheck.cpp

292–293

These can be combined into a single if statement.

300–301

Why is this split into two lines?

304

This continue can be dropped without changing the semantics, correct?

clang-tidy/bugprone/ArgumentCommentCheck.h

44–46

Why not character or string literals?
What about nullptr literals or UDLs?

docs/clang-tidy/checks/bugprone-argument-comment.rst

40

Format the code examples from our style guide as well (same below).

[MyDeveloperDay]

Address review comments
Add support for additional StringLiterals,CharacterLiterals,UserDefinedLiterals and NullPtrs
Simplify the Options names from "AddCommentsToXXX" to "CommentXXX"
Add extra unit tests to support additional supported literal types

[aaron.ballman]

clang-tidy/bugprone/ArgumentCommentCheck.cpp

219–227

What's going on with these? Why not return isa<Blah>(Arg->IgnoreImpCasts()); (at which point, no need for the separate functions).

293–299

This is a bit... much. I'm not certain what would be a better approach, but at the very least, factoring this out into a simple function call should make it a bit less... much. :-)

clang-tidy/bugprone/ArgumentCommentCheck.h

43–47

At this point, I think it would be better to turn these into: const unsigned Blah : 1;

docs/clang-tidy/checks/bugprone-argument-comment.rst

40

This still seems to be happening in the current revision?

[MyDeveloperDay]

Address review comments

clang-tidy/bugprone/ArgumentCommentCheck.cpp

219–227

OK, my bad, I was just looking at the ast-dump on godbolt.org thinking... how do I get past that ImplicitCasrExpr, learning these tricks in the AST isn't always obvious despite me looking in doxygen, when you don't know what to look for its hard to know..but this is a neat trick and I'm happy to learn.

docs/clang-tidy/checks/bugprone-argument-comment.rst

40

Sorry didn't catch your meaning but I assume it was the space between the arguments...

[MyDeveloperDay]

clang-tidy/bugprone/ArgumentCommentCheck.cpp

236

@aaron.ballman, slight aside (and not in the code I introduced), when I run clang-tidy on the code I'm sending for review, I get the following...

C:\clang\llvm\tools\clang\tools\extra\clang-tidy\bugprone\ArgumentCommentCheck.cpp:253:8: warning: invalid case style for variable 'makeFileCharRange' [readability-identifier-naming]
  auto makeFileCharRange = [Ctx](SourceLocation Begin, SourceLocation End) {
       ^~~~~~~~~~~~~~~~~
       MakeFileCharRange

according to the .clang-tidy, a variable should be CamelCase, and a function camelBack, as its a Lambda what do we consider it should be? and does this really require an improvement in readability-identifier-naming? (might be something I'd be happy to look at next)

Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,readability-identifier-naming'
CheckOptions:
  - key:             readability-identifier-naming.ClassCase
    value:           CamelCase
  - key:             readability-identifier-naming.EnumCase
    value:           CamelCase
  - key:             readability-identifier-naming.FunctionCase
    value:           camelBack
  - key:             readability-identifier-naming.MemberCase
    value:           CamelCase
  - key:             readability-identifier-naming.ParameterCase
    value:           CamelCase
  - key:             readability-identifier-naming.UnionCase
    value:           CamelCase
  - key:             readability-identifier-naming.VariableCase
    value:           CamelCase

[aaron.ballman]

clang-tidy/bugprone/ArgumentCommentCheck.cpp

1

Why did this get removed?

219–227

No worries, I was just wondering if there was something special about a FullExpr that you didn't want to look though it for some reason. Glad to show you a new trick!

222

I think we may want to do some additional work here. Consider:

#define FOO 1

void g(int);
void h(double);
void i(const char *);

void f() {
  g(FOO); // Probably don't want to suggest comments here
  g((1)); // Probably do want to suggest comments here
  h(1.0f); // Probably do want to suggest comments here
  i(__FILE__); // Probably don't want to suggest comments here
}

I think this code should do two things: 1) check whether the location of the arg is a macro expansion (if so, return false), 2) do Arg = Arg->IgnoreParenImpCasts(); at the start of the function and drop the Arg->IgnoreImpCasts() for the string and pointer literal cases. However, that may be a bridge too far, so you should add a test case like this:

g(_Generic(0, int : 0)); // Definitely do not want to see comments here

If it turns out the above case gives the comment suggestions, then I'd go with IgnoreImpCasts() rather than IgnoreParenImpCasts().

236

according to the .clang-tidy, a variable should be CamelCase, and a function camelBack, as its a Lambda what do we consider it should be? and does this really require an improvement in readability-identifier-naming? (might be something I'd be happy to look at next)

Lambdas are variables, so I would expect those to follow the variable naming conventions. This is consistent with how we treat variables of function pointer type as well.

clang-tidy/bugprone/ArgumentCommentCheck.h

43–46

Can you include this in the bit-field packing as well (with type unsigned)?

docs/clang-tidy/checks/bugprone-argument-comment.rst

40

Yup, it was!

[MyDeveloperDay]

-Address review comments
-Ignore argument comment addition for macro inputs
-Add unit tests to cover the above
-Fix clang-tidy suggestion (from readability-identifier-naming)
-Ensure file is clang-formatted

[MyDeveloperDay]

clang-tidy/bugprone/ArgumentCommentCheck.cpp

1

Sorry I suspect poor vim skills

222

I agree, tried all combinations but g((1); and g(_Generic(0, int : 0)); would otherwise get comments

[aaron.ballman]

LGTM aside from a testing nit.

clang-tidy/bugprone/ArgumentCommentCheck.cpp

222

Yeah, I kind of figured that would be the case. Thank you for checking!

test/clang-tidy/bugprone-argument-comment-literals.cpp

105

Can you add a FIXME comment that says we'd like to handle this case at some point? Maybe move the test closer to the _Generic example so you can mention that case as being a problem when handling paren expressions.

[MyDeveloperDay]

Pre-Commit Changes

• add FIXME wording to test
• move local header include after libraries
• rebase

clang-tidy/bugprone/ArgumentCommentCheck.h

 /// The check understands argument comments in the form `/*parameter_name=*/`
 /// that are placed right before the argument.
 ///
 /// \code
 ///   void f(bool foo);
 ///
 ///   ...
 ///   f(/*bar=*/true);
-///   // warning: argument name 'bar' in comment does not match parameter name 'foo'
+///   // warning: argument name 'bar' in comment does not match parameter name
+///   'foo'
 /// \endcode
 ///
 /// 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;
+  const bool AddCommentsToBoolLiterals;
+  const bool AddCommentsToIntegerLiterals;
+  const bool AddCommentsToFloatLiterals;
   llvm::Regex IdentRE;
 
   void checkCallArgs(ASTContext *Ctx, const FunctionDecl *Callee,
                      SourceLocation ArgBeginLoc,
                      llvm::ArrayRef<const Expr *> Args);
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ARGUMENTCOMMENTCHECK_H

clang-tidy/bugprone/ArgumentCommentCheck.cpp

 //===--- ArgumentCommentCheck.cpp - clang-tidy ----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ArgumentCommentCheck.h"
+#include "../utils/LexerUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Token.h"
-#include "../utils/LexerUtils.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 ArgumentCommentCheck::ArgumentCommentCheck(StringRef Name,
                                            ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       StrictMode(Options.getLocalOrGlobal("StrictMode", 0) != 0),
+      AddCommentsToBoolLiterals(
+          Options.getLocalOrGlobal("AddCommentsToBoolLiterals", 0) != 0),
+      AddCommentsToIntegerLiterals(
+          Options.getLocalOrGlobal("AddCommentsToIntegerLiterals", 0) != 0),
+      AddCommentsToFloatLiterals(
+          Options.getLocalOrGlobal("AddCommentsToFloatLiterals", 0) != 0),
       IdentRE("^(/\\* *)([_A-Za-z][_A-Za-z0-9]*)( *= *\\*/)$") {}
 
 void ArgumentCommentCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "StrictMode", StrictMode);
+  Options.store(Opts, "AddCommentsToBoolLiterals", AddCommentsToBoolLiterals);
+  Options.store(Opts, "AddCommentsToIntegerLiterals",
+                AddCommentsToIntegerLiterals);
+  Options.store(Opts, "AddCommentsToFloatLiterals", AddCommentsToFloatLiterals);
 }
 
 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.
if (looksLikeExpectMethod(Method)) {
     for (const auto *D : Ctx->decls()) {
       if (D->getNextDeclInContext() == Method) {
         const auto *Previous = dyn_cast<CXXMethodDecl>(D);
         return areMockAndExpectMethods(Previous, Method) ? Previous : nullptr;
       }
     }
     return nullptr;
   }
-  if (const auto *Next = dyn_cast_or_null<CXXMethodDecl>(
-                 Method->getNextDeclInContext())) {
+  if (const auto *Next =
+          dyn_cast_or_null<CXXMethodDecl>(Method->getNextDeclInContext())) {
     if (looksLikeExpectMethod(Next) && areMockAndExpectMethods(Method, Next))
       return Method;
   }
   return nullptr;
 }
 
 // For gmock expectation builder method (the target of the call generated by
 // `EXPECT_CALL(obj, Method(...))`) tries to find the real method being mocked
 // (returns nullptr, if the mock method doesn't override anything). For other
 // functions returns the function itself.
 static const FunctionDecl *resolveMocks(const FunctionDecl *Func) {
   if (const auto *Method = dyn_cast<CXXMethodDecl>(Func)) {
     if (const auto *MockedMethod = findMockedMethod(Method)) {
       // If mocked method overrides the real one, we can use its parameter
       // names, otherwise we're out of luck.
       if (MockedMethod->size_overridden_methods() > 0) {
         return *MockedMethod->begin_overridden_methods();
       }
       return nullptr;
     }
   }
   return Func;
 }
 
 void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx,
                                          const FunctionDecl *OriginalCallee,
                                          SourceLocation ArgBeginLoc,
                                          llvm::ArrayRef<const Expr *> Args) {
   const FunctionDecl *Callee = resolveMocks(OriginalCallee);
   if (!Callee)
     return;
 
   Callee = Callee->getFirstDecl();
   unsigned NumArgs = std::min<unsigned>(Args.size(), Callee->getNumParams());
   if (NumArgs == 0)
     return;
 
   auto makeFileCharRange = [Ctx](SourceLocation Begin, SourceLocation End) {
     return Lexer::makeFileCharRange(CharSourceRange::getCharRange(Begin, End),
                                     Ctx->getSourceManager(),
                                     Ctx->getLangOpts());
   };
 
   for (unsigned I = 0; I < NumArgs; ++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
for (auto Comment : Comments) {
         diag(PVD->getLocation(), "%0 declared here", DiagnosticIDs::Note) << II;
         if (OriginalCallee != Callee) {
           diag(OriginalCallee->getLocation(),
                "actual callee (%0) is declared here", DiagnosticIDs::Note)
               << OriginalCallee;
         }
       }
     }
+
+    // If the argument comments are missing for literals add them.
+    if (Comments.empty()) {
+      if (((isa<CXXBoolLiteralExpr>(Args[I]) && AddCommentsToBoolLiterals) ||
+           (isa<IntegerLiteral>(Args[I]) && AddCommentsToIntegerLiterals) ||
+           (isa<FloatingLiteral>(Args[I]) && AddCommentsToFloatLiterals))) {
+        std::string ArgComment =
+            (llvm::Twine("/*") + II->getName() + "=*/").str();
+        DiagnosticBuilder Diag =
+            diag(Args[I]->getBeginLoc(),
+                 "argument comment missing for literal argument"
+                 " %0")
+            << II
+            << FixItHint::CreateInsertion(Args[I]->getBeginLoc(), ArgComment);
+        continue;
+      }
+    }
   }
 }
 
 void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *E = Result.Nodes.getNodeAs<Expr>("expr");
   if (const auto *Call = dyn_cast<CallExpr>(E)) {
     const FunctionDecl *Callee = Call->getDirectCallee();
     if (!Callee)

docs/ReleaseNotes.rst

- New :doc:`abseil-duration-addition
   domain.
 
 - New :doc:`abseil-duration-conversion-cast
   <clang-tidy/checks/abseil-duration-conversion-cast>` check.
 
   Checks for casts of ``absl::Duration`` conversion functions, and recommends
   the right conversion function instead.
 
+- The :doc:`bugprone-argument-comment
+  <clang-tidy/checks/bugprone-argument-comment>` now supports 
+  `AddCommentsToBoolLiterals`, `AddCommentsToIntegerLiterals` &
+  `AddCommentsToFloatLiterals` options.
+
 - New :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
   check.
 
   Checks whether there are underscores in googletest test and test case names in
   test macros, which is prohibited by the Googletest FAQ.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/bugprone-argument-comment.rst

 Options
 -------
 
 .. option:: StrictMode
 
    When zero (default value), the check will ignore leading and trailing
    underscores and case when comparing names -- otherwise they are taken into
    account.
+
+.. option:: AddCommentsToBoolLiterals
+
+   When true, the check will add argument comments in the format
+   ``/*parameter_name=*/`` right before the boolean literal argument.
+
+Before:
+
+.. code-block:: c++
+
+  void foo(bool turn_key,bool press_button);
+
+  foo(true,false);
+
+After:
+
+.. code-block:: c++
+
+  void foo(bool turn_key,bool press_button);
+
+  foo(/*turn_key=*/true,/*press_button*/false);
+
+.. option:: AddCommentsToIntegerLiterals
+
+   When true, the check will add argument comments in the format
+   ``/*parameter_name=*/`` right before the integer literal argument.
+
+Before:
+
+.. code-block:: c++
+
+  void foo(int meaning_of_life);
+
+  foo(42);
+
+After:
+
+.. code-block:: c++
+
+  void foo(int meaning_of_life);
+
+  foo(/*meaning_of_life=*/42);
+
+.. option:: AddCommentsToFloatLiterals
+
+   When true, the check will add argument comments in the format
+   ``/*parameter_name=*/`` right before the float/double literal argument.
+
+Before:
+
+.. code-block:: c++
+
+  void foo(float pi);
+
+  foo(3.14159);
+
+After:
+
+.. code-block:: c++
+
+  void foo(float pi);
+
+  foo(/*pi=*/3.14159);

test/clang-tidy/bugprone-argument-comment-literals.cpp

+// RUN: %check_clang_tidy %s bugprone-argument-comment %t -- \
+// RUN:   -config="{CheckOptions: [{key: AddCommentsToBoolLiterals, value: 1},{key: AddCommentsToIntegerLiterals, value: 1},{key: AddCommentsToFloatLiterals, value: 1}]}" --
+
+struct A {
+  void foo(bool abc);
+  void foo(bool abc, bool cde);
+  void foo(const char *, bool abc);
+  void foo(int iabc);
+  void foo(float fabc);
+  void foo(double dabc);
+};
+
+void test() {
+  A a;
+
+  a.foo(true);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'abc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*abc=*/true);
+
+  a.foo(false);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'abc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*abc=*/false);
+
+  a.foo(true, false);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'abc' [bugprone-argument-comment]
+  // CHECK-MESSAGES: [[@LINE-2]]:15: warning: argument comment missing for literal argument 'cde' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*abc=*/true, /*cde=*/false);
+
+  a.foo(false, true);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'abc' [bugprone-argument-comment]
+  // CHECK-MESSAGES: [[@LINE-2]]:16: warning: argument comment missing for literal argument 'cde' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*abc=*/false, /*cde=*/true);
+
+  a.foo(/*abc=*/false, true);
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: argument comment missing for literal argument 'cde' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*abc=*/false, /*cde=*/true);
+
+  a.foo(false, /*cde=*/true);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'abc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*abc=*/false, /*cde=*/true);
+
+  bool val1 = true;
+  bool val2 = false;
+  a.foo(val1, val2);
+
+  a.foo("", true);
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: argument comment missing for literal argument 'abc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo("", /*abc=*/true);
+
+  a.foo(0);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'iabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*iabc=*/0);
+
+  a.foo(1.0f);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'fabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*fabc=*/1.0f);
+
+  a.foo(1.0);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'dabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*dabc=*/1.0);
+
+  int val3 = 10;
+  a.foo(val3);
+
+  float val4 = 10.0;
+  a.foo(val4);
+
+  double val5 = 10.0;
+  a.foo(val5);
+}
+
+void f(bool _with_underscores_);
+void ignores_underscores() {
+  f(false);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: argument comment missing for literal argument '_with_underscores_' [bugprone-argument-comment]
+  // CHECK-FIXES: f(/*_with_underscores_=*/false);
+
+  f(true);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: argument comment missing for literal argument
+  // CHECK-FIXES: f(/*_with_underscores_=*/true);
+}