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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
