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

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL353535: [clang-tidy] Add options to 
bugprone-argument-comment to add missing argument… (authored by paulhoad, 
committed by ).
Herald added a project: LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D57674?vs=185947=185985#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57674

Files:
  clang-tools-extra/trunk/clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tools-extra/trunk/clang-tidy/bugprone/ArgumentCommentCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-argument-comment.rst
  clang-tools-extra/trunk/test/clang-tidy/bugprone-argument-comment-literals.cpp

Index: clang-tools-extra/trunk/clang-tidy/bugprone/ArgumentCommentCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/ArgumentCommentCheck.h
+++ clang-tools-extra/trunk/clang-tidy/bugprone/ArgumentCommentCheck.h
@@ -26,7 +26,8 @@
 ///
 ///   ...
 ///   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.
@@ -39,12 +40,21 @@
   void storeOptions(ClangTidyOptions::OptionMap ) override;
 
 private:
-  const bool StrictMode;
+  const unsigned StrictMode : 1;
+  const unsigned CommentBoolLiterals : 1;
+  const unsigned CommentIntegerLiterals : 1;
+  const unsigned CommentFloatLiterals : 1;
+  const unsigned CommentStringLiterals : 1;
+  const unsigned CommentUserDefinedLiterals : 1;
+  const unsigned CommentCharacterLiterals : 1;
+  const unsigned CommentNullPtrs : 1;
   llvm::Regex IdentRE;
 
   void checkCallArgs(ASTContext *Ctx, const FunctionDecl *Callee,
  SourceLocation ArgBeginLoc,
  llvm::ArrayRef Args);
+
+  bool shouldAddComment(const Expr *Arg) const;
 };
 
 } // namespace bugprone
Index: clang-tools-extra/trunk/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -11,6 +11,7 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Token.h"
+
 #include "../utils/LexerUtils.h"
 
 using namespace clang::ast_matchers;
@@ -23,17 +24,37 @@
ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
   StrictMode(Options.getLocalOrGlobal("StrictMode", 0) != 0),
+  CommentBoolLiterals(Options.getLocalOrGlobal("CommentBoolLiterals", 0) !=
+  0),
+  CommentIntegerLiterals(
+  Options.getLocalOrGlobal("CommentIntegerLiterals", 0) != 0),
+  CommentFloatLiterals(
+  Options.getLocalOrGlobal("CommentFloatLiterals", 0) != 0),
+  CommentStringLiterals(
+  Options.getLocalOrGlobal("CommentStringLiterals", 0) != 0),
+  CommentUserDefinedLiterals(
+  Options.getLocalOrGlobal("CommentUserDefinedLiterals", 0) != 0),
+  CommentCharacterLiterals(
+  Options.getLocalOrGlobal("CommentCharacterLiterals", 0) != 0),
+  CommentNullPtrs(Options.getLocalOrGlobal("CommentNullPtrs", 0) != 0),
   IdentRE("^(/\\* *)([_A-Za-z][_A-Za-z0-9]*)( *= *\\*/)$") {}
 
 void ArgumentCommentCheck::storeOptions(ClangTidyOptions::OptionMap ) {
   Options.store(Opts, "StrictMode", StrictMode);
+  Options.store(Opts, "CommentBoolLiterals", CommentBoolLiterals);
+  Options.store(Opts, "CommentIntegerLiterals", CommentIntegerLiterals);
+  Options.store(Opts, "CommentFloatLiterals", CommentFloatLiterals);
+  Options.store(Opts, "CommentStringLiterals", CommentStringLiterals);
+  Options.store(Opts, "CommentUserDefinedLiterals", CommentUserDefinedLiterals);
+  Options.store(Opts, "CommentCharacterLiterals", CommentCharacterLiterals);
+  Options.store(Opts, "CommentNullPtrs", CommentNullPtrs);
 }
 
 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.
+   // NewCallback's arguments relate to the pointed function,
+   // don't check them against NewCallback's parameter names.
// FIXME: Make this configurable.
unless(hasDeclaration(functionDecl(
hasAnyName("NewCallback", "NewPermanentCallback")
@@ -126,8 +147,8 @@
 
 const unsigned Threshold = 2;
 // Other parameters must be an edit distance at least 

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 185947.
MyDeveloperDay marked 2 inline comments as done.
MyDeveloperDay added a comment.

Pre-Commit Changes

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


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

https://reviews.llvm.org/D57674

Files:
  clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tidy/bugprone/ArgumentCommentCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-argument-comment.rst
  test/clang-tidy/bugprone-argument-comment-literals.cpp

Index: test/clang-tidy/bugprone-argument-comment-literals.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-argument-comment-literals.cpp
@@ -0,0 +1,124 @@
+// RUN: %check_clang_tidy %s bugprone-argument-comment %t -- \
+// RUN:   -config="{CheckOptions: [{key: CommentBoolLiterals, value: 1},{key: CommentIntegerLiterals, value: 1}, {key: CommentFloatLiterals, value: 1}, {key: CommentUserDefinedLiterals, value: 1}, {key: CommentStringLiterals, value: 1}, {key: CommentNullPtrs, value: 1}, {key: CommentCharacterLiterals, 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 foo(const char *strabc);
+  void fooW(const wchar_t *wstrabc);
+  void fooPtr(A *ptrabc);
+  void foo(char chabc);
+};
+
+#define FOO 1
+
+void g(int a);
+void h(double b);
+void i(const char *c);
+
+double operator"" _km(long double);
+
+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);
+
+  a.foo("Hello World");
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'strabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*strabc=*/"Hello World");
+  //
+  a.fooW(L"Hello World");
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: argument comment missing for literal argument 'wstrabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.fooW(/*wstrabc=*/L"Hello World");
+
+  a.fooPtr(nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: argument comment missing for literal argument 'ptrabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.fooPtr(/*ptrabc=*/nullptr);
+
+  a.foo(402.0_km);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'dabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*dabc=*/402.0_km);
+
+  a.foo('A');
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'chabc' [bugprone-argument-comment]
+  // 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from a testing nit.




Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:231
+bool ArgumentCommentCheck::shouldAddComment(const Expr *Arg) const {
+  return (CommentBoolLiterals && isa(Arg)) ||
+ (CommentIntegerLiterals && isa(Arg)) ||

MyDeveloperDay wrote:
> aaron.ballman wrote:
> > 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()`.
> I agree, tried all combinations but g((1); and g(_Generic(0, int : 0)); would 
> otherwise get comments
Yeah, I kind of figured that would be the case. Thank you for checking!



Comment at: test/clang-tidy/bugprone-argument-comment-literals.cpp:104
+  g(FOO);
+  g((1));
+  h(1.0f);

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.


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

https://reviews.llvm.org/D57674



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added inline comments.



Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:1
-//===--- ArgumentCommentCheck.cpp - clang-tidy 
===//
 //

aaron.ballman wrote:
> Why did this get removed?
Sorry I suspect poor vim skills



Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:231
+bool ArgumentCommentCheck::shouldAddComment(const Expr *Arg) const {
+  return (CommentBoolLiterals && isa(Arg)) ||
+ (CommentIntegerLiterals && isa(Arg)) ||

aaron.ballman wrote:
> 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()`.
I agree, tried all combinations but g((1); and g(_Generic(0, int : 0)); would 
otherwise get comments


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

https://reviews.llvm.org/D57674



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 185632.
MyDeveloperDay marked 5 inline comments as done.
MyDeveloperDay added a comment.

-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


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

https://reviews.llvm.org/D57674

Files:
  clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tidy/bugprone/ArgumentCommentCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-argument-comment.rst
  test/clang-tidy/bugprone-argument-comment-literals.cpp

Index: test/clang-tidy/bugprone-argument-comment-literals.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-argument-comment-literals.cpp
@@ -0,0 +1,122 @@
+// RUN: %check_clang_tidy %s bugprone-argument-comment %t -- \
+// RUN:   -config="{CheckOptions: [{key: CommentBoolLiterals, value: 1},{key: CommentIntegerLiterals, value: 1}, {key: CommentFloatLiterals, value: 1}, {key: CommentUserDefinedLiterals, value: 1}, {key: CommentStringLiterals, value: 1}, {key: CommentNullPtrs, value: 1}, {key: CommentCharacterLiterals, 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 foo(const char *strabc);
+  void fooW(const wchar_t *wstrabc);
+  void fooPtr(A *ptrabc);
+  void foo(char chabc);
+};
+
+#define FOO 1
+
+void g(int a);
+void h(double b);
+void i(const char *c);
+
+double operator"" _km(long double);
+
+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);
+
+  a.foo("Hello World");
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'strabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*strabc=*/"Hello World");
+  //
+  a.fooW(L"Hello World");
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: argument comment missing for literal argument 'wstrabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.fooW(/*wstrabc=*/L"Hello World");
+
+  a.fooPtr(nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: argument comment missing for literal argument 'ptrabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.fooPtr(/*ptrabc=*/nullptr);
+
+  a.foo(402.0_km);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'dabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*dabc=*/402.0_km);
+
+  a.foo('A');
+  // CHECK-MESSAGES: 

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

[Aaron Ballman via Phabricator via cfe-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(Arg);
+  return Cast ? isa(Cast->getSubExpr()) : false;
+}
+
+static bool isNullPtrLiteral(const Expr *Arg) {
+  const auto *Cast = dyn_cast(Arg);

MyDeveloperDay wrote:
> aaron.ballman wrote:
> > What's going on with these? Why not `return 
> > isa(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(Arg)) ||
+ (CommentIntegerLiterals && isa(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: 

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added inline comments.



Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:257
 Ctx->getLangOpts());
   };
 

@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
```










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

https://reviews.llvm.org/D57674



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 185385.
MyDeveloperDay marked 6 inline comments as done.
MyDeveloperDay added a comment.

Address review comments


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

https://reviews.llvm.org/D57674

Files:
  clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tidy/bugprone/ArgumentCommentCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-argument-comment.rst
  test/clang-tidy/bugprone-argument-comment-literals.cpp

Index: test/clang-tidy/bugprone-argument-comment-literals.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-argument-comment-literals.cpp
@@ -0,0 +1,107 @@
+// RUN: %check_clang_tidy %s bugprone-argument-comment %t -- \
+// RUN:   -config="{CheckOptions: [{key: CommentBoolLiterals, value: 1},{key: CommentIntegerLiterals, value: 1}, {key: CommentFloatLiterals, value: 1}, {key: CommentUserDefinedLiterals, value: 1}, {key: CommentStringLiterals, value: 1}, {key: CommentNullPtrs, value: 1}, {key: CommentCharacterLiterals, 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 foo(const char *strabc);
+  void fooW(const wchar_t *wstrabc);
+  void fooPtr(A *ptrabc);
+  void foo(char chabc);
+};
+
+double operator"" _km(long double);
+
+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);
+
+  a.foo("Hello World");
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'strabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*strabc=*/"Hello World");
+  //
+  a.fooW(L"Hello World");
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: argument comment missing for literal argument 'wstrabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.fooW(/*wstrabc=*/L"Hello World");
+
+  a.fooPtr(nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: argument comment missing for literal argument 'ptrabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.fooPtr(/*ptrabc=*/nullptr);
+
+  a.foo(402.0_km);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'dabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*dabc=*/402.0_km);
+
+  a.foo('A');
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'chabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*chabc=*/'A');
+}
+
+void f(bool _with_underscores_);
+void ignores_underscores() {
+  f(false);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: 

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:228-236
+static bool isStringLiteral(const Expr *Arg) {
+  const auto *Cast = dyn_cast(Arg);
+  return Cast ? isa(Cast->getSubExpr()) : false;
+}
+
+static bool isNullPtrLiteral(const Expr *Arg) {
+  const auto *Cast = dyn_cast(Arg);

aaron.ballman wrote:
> What's going on with these? Why not `return 
> isa(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.



Comment at: docs/clang-tidy/checks/bugprone-argument-comment.rst:40
+
+  void foo(bool turn_key,bool press_button);
+

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...


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

https://reviews.llvm.org/D57674



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-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(Arg);
+  return Cast ? isa(Cast->getSubExpr()) : false;
+}
+
+static bool isNullPtrLiteral(const Expr *Arg) {
+  const auto *Cast = dyn_cast(Arg);

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



Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:312-318
+(((CommentBoolLiterals && isa(Args[I])) ||
+  (CommentIntegerLiterals && isa(Args[I])) ||
+  (CommentFloatLiterals && isa(Args[I])) ||
+  (CommentUserDefinedLiterals && isa(Args[I])) ||
+  (CommentCharacterLiterals && isa(Args[I])) ||
+  (CommentStringLiterals && isStringLiteral(Args[I])) ||
+  (CommentNullPtrs && isNullPtrLiteral(Args[I]) {

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. :-)



Comment at: clang-tidy/bugprone/ArgumentCommentCheck.h:43-50
   const bool StrictMode;
+  const bool CommentBoolLiterals;
+  const bool CommentIntegerLiterals;
+  const bool CommentFloatLiterals;
+  const bool CommentStringLiterals;
+  const bool CommentUserDefinedLiterals;
+  const bool CommentCharacterLiterals;

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



Comment at: docs/clang-tidy/checks/bugprone-argument-comment.rst:40
+
+  void foo(bool turn_key,bool press_button);
+

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?


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

https://reviews.llvm.org/D57674



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 185148.
MyDeveloperDay edited the summary of this revision.
MyDeveloperDay added a comment.

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


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

https://reviews.llvm.org/D57674

Files:
  clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tidy/bugprone/ArgumentCommentCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-argument-comment.rst
  test/clang-tidy/bugprone-argument-comment-literals.cpp

Index: test/clang-tidy/bugprone-argument-comment-literals.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-argument-comment-literals.cpp
@@ -0,0 +1,107 @@
+// RUN: %check_clang_tidy %s bugprone-argument-comment %t -- \
+// RUN:   -config="{CheckOptions: [{key: CommentBoolLiterals, value: 1},{key: CommentIntegerLiterals, value: 1}, {key: CommentFloatLiterals, value: 1}, {key: CommentUserDefinedLiterals, value: 1}, {key: CommentStringLiterals, value: 1}, {key: CommentNullPtrs, value: 1}, {key: CommentCharacterLiterals, 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 foo(const char *strabc);
+  void fooW(const wchar_t *wstrabc);
+  void fooPtr(A *ptrabc);
+  void foo(char chabc);
+};
+
+double operator"" _km(long double);
+
+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);
+
+  a.foo("Hello World");
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'strabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*strabc=*/"Hello World");
+  //
+  a.fooW(L"Hello World");
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: argument comment missing for literal argument 'wstrabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.fooW(/*wstrabc=*/L"Hello World");
+
+  a.fooPtr(nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: argument comment missing for literal argument 'ptrabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.fooPtr(/*ptrabc=*/nullptr);
+
+  a.foo(402.0_km);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'dabc' [bugprone-argument-comment]
+  // CHECK-FIXES: a.foo(/*dabc=*/402.0_km);
+
+  a.foo('A');
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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




Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:292-293
+// If the argument comments are missing for literals add them.
+if (Comments.empty()) {
+  if (((isa(Args[I]) && AddCommentsToBoolLiterals) ||
+   (isa(Args[I]) && AddCommentsToIntegerLiterals) ||

These can be combined into a single `if` statement.



Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:300-301
+diag(Args[I]->getBeginLoc(),
+ "argument comment missing for literal argument"
+ " %0")
+<< II

Why is this split into two lines?



Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:304
+<< FixItHint::CreateInsertion(Args[I]->getBeginLoc(), ArgComment);
+continue;
+  }

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



Comment at: clang-tidy/bugprone/ArgumentCommentCheck.h:44-46
+  const bool AddCommentsToBoolLiterals;
+  const bool AddCommentsToIntegerLiterals;
+  const bool AddCommentsToFloatLiterals;

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



Comment at: docs/clang-tidy/checks/bugprone-argument-comment.rst:40
+
+  void foo(bool turn_key,bool press_button);
+

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


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

https://reviews.llvm.org/D57674



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:82
 
+- The :doc:`bugprone-argument-comment
+  ` now supports 

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


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

https://reviews.llvm.org/D57674



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 185014.
MyDeveloperDay added a comment.

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


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

https://reviews.llvm.org/D57674

Files:
  clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tidy/bugprone/ArgumentCommentCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-argument-comment.rst
  test/clang-tidy/bugprone-argument-comment-literals.cpp

Index: test/clang-tidy/bugprone-argument-comment-literals.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-argument-comment-literals.cpp
@@ -0,0 +1,81 @@
+// 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);
+}
Index: docs/clang-tidy/checks/bugprone-argument-comment.rst
===
--- docs/clang-tidy/checks/bugprone-argument-comment.rst
+++ docs/clang-tidy/checks/bugprone-argument-comment.rst
@@ -27,3 +27,66 @@
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 

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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 



Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57674



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added inline comments.



Comment at: docs/ReleaseNotes.rst:89
 
+- The :doc:`bugprone-argument-comment
+  ` now supports 

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


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57674



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: JonasToth, alexfh, hokein, aaron.ballman, 
Eugene.Zelenko.
MyDeveloperDay added a project: clang-tools-extra.
Herald added a subscriber: xazax.hun.
Herald added a project: clang.

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.AddCommentsToBoolLiterals
  value:   '1'
- key: bugprone-argument-comment.AddCommentsToFloatLiterals
  value:   '1'
- key: bugprone-argument-comment.AddCommentsToIntegerLiterals
  value:   '1'

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D57674

Files:
  clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tidy/bugprone/ArgumentCommentCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-argument-comment.rst
  test/clang-tidy/bugprone-argument-comment-literals.cpp

Index: test/clang-tidy/bugprone-argument-comment-literals.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-argument-comment-literals.cpp
@@ -0,0 +1,81 @@
+// 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);
+}
Index: docs/clang-tidy/checks/bugprone-argument-comment.rst
===
--- docs/clang-tidy/checks/bugprone-argument-comment.rst
+++ docs/clang-tidy/checks/bugprone-argument-comment.rst
@@ -27,3 +27,18 @@
When