[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 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<CXXBoolLiteralExpr>(Arg)) ||
+         (CommentIntegerLiterals && isa<IntegerLiteral>(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