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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:228-236
+static bool isStringLiteral(const Expr *Arg) {
+  const auto *Cast = dyn_cast<ImplicitCastExpr>(Arg);
+  return Cast ? isa<StringLiteral>(Cast->getSubExpr()) : false;
+}
+
+static bool isNullPtrLiteral(const Expr *Arg) {
+  const auto *Cast = dyn_cast<ImplicitCastExpr>(Arg);
----------------
MyDeveloperDay wrote:
> aaron.ballman wrote:
> > What's going on with these? Why not `return isa<Blah>(Arg->IgnoreImpCasts());` (at which point, no need for the separate functions).
> 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.
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!


================
Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:1
-//===--- ArgumentCommentCheck.cpp - clang-tidy ----------------------------===//
 //
----------------
Why did this get removed?


================
Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:231
+bool ArgumentCommentCheck::shouldAddComment(const Expr *Arg) const {
+  return (CommentBoolLiterals && isa<CXXBoolLiteralExpr>(Arg)) ||
+         (CommentIntegerLiterals && isa<IntegerLiteral>(Arg)) ||
----------------
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()`.


================
Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:257
                                     Ctx->getLangOpts());
   };
 
----------------
MyDeveloperDay wrote:
> @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
> ```
> 
> 
> 
> 
> 
> 
> 
> 
> 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.


================
Comment at: clang-tidy/bugprone/ArgumentCommentCheck.h:43
 private:
   const bool StrictMode;
+  const unsigned CommentBoolLiterals : 1;
----------------
Can you include this in the bit-field packing as well (with type `unsigned`)?


================
Comment at: docs/clang-tidy/checks/bugprone-argument-comment.rst:40
+
+  void foo(bool turn_key,bool press_button);
+
----------------
MyDeveloperDay wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > Format the code examples from our style guide as well (same below).
> > This still seems to be happening in the current revision?
> Sorry didn't catch your meaning but I assume it was the space between the arguments...
Yup, it was!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57674/new/

https://reviews.llvm.org/D57674