[clang-tidy] Fix bugprone-argument-comment bug: negative literal number is not checked.

[xyb]

Summary

For example:

void foo(int a);
foo(-2);

should be fixed as:

foo(/*a=*/-2);

This change tries to fix this issue.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp

240 ↗

(On Diff #218386)

The bug claims that this is only for handling negative literals, but this allows any unary operator. Is that intentional? If we're going to allow arbitrary unary operators, why not arbitrary binary operators as well?

[xyb]

clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp

240 ↗

(On Diff #218386)

Actually, it handles "UnaryOperator Literal". So it will handle "+12", "-12", "~12". I can restrict it for UO_MINUS only, but is it real necessary?

[alexfh]

Please add full context to the diff. See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[xyb]

Update with full diff.

[alexfh]

LG if there are no other concerns.

[aaron.ballman]

LGTM!

clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp

240 ↗

(On Diff #218386)

It's not that it's necessary, it's that I'm wondering why we think -12 is special but 0 - 12 is not, but that can be done incrementally later.

[xyb]

Thanks. BTW, I can't commit the patch by myself.

[alexfh]

In D67084#1657534, @xyb wrote:

Thanks. BTW, I can't commit the patch by myself.

The patch doesn't apply cleanly. Please rebase it against current HEAD.

[xyb]

Rebase patch with current HEAD.

clang-tools-extra/trunk/clang-tidy/bugprone/ArgumentCommentCheck.cpp

if (const auto *Method = dyn_cast<CXXMethodDecl>(Func)) {
     }
   }
   return Func;
 }
 
 // Given the argument type and the options determine if we should
 // be adding an argument comment.
 bool ArgumentCommentCheck::shouldAddComment(const Expr *Arg) const {
+  Arg = Arg->IgnoreImpCasts();
+  if (isa<UnaryOperator>(Arg))
+    Arg = cast<UnaryOperator>(Arg)->getSubExpr();
   if (Arg->getExprLoc().isMacroID())
     return false;
-  Arg = Arg->IgnoreImpCasts();
   return (CommentBoolLiterals && isa<CXXBoolLiteralExpr>(Arg)) ||
          (CommentIntegerLiterals && isa<IntegerLiteral>(Arg)) ||
          (CommentFloatLiterals && isa<FloatingLiteral>(Arg)) ||
          (CommentUserDefinedLiterals && isa<UserDefinedLiteral>(Arg)) ||
          (CommentCharacterLiterals && isa<CharacterLiteral>(Arg)) ||
          (CommentStringLiterals && isa<StringLiteral>(Arg)) ||
          (CommentNullPtrs && isa<CXXNullPtrLiteralExpr>(Arg));
 }

clang-tools-extra/trunk/test/clang-tidy/bugprone-argument-comment-literals.cpp

void test() {
   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.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);
 
+  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);
+  a.foo(-val3);
 
   float val4 = 10.0;
   a.foo(val4);
+  a.foo(-val4);
 
   double val5 = 10.0;
   a.foo(val5);
+  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(-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');
 
   g(FOO);
+  g(-FOO);
   h(1.0f);
   // CHECK-MESSAGES: [[@LINE-1]]:5: warning: argument comment missing for literal argument 'b' [bugprone-argument-comment]
   // CHECK-FIXES: h(/*b=*/1.0f);
+  h(-1.0f);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: argument comment missing for literal argument 'b' [bugprone-argument-comment]
+  // CHECK-FIXES: h(/*b=*/-1.0f);
   i(__FILE__);
 
   j(1, X(1), X(1));
   // CHECK-MESSAGES: [[@LINE-1]]:5: warning: argument comment missing for literal argument 'a' [bugprone-argument-comment]
   // CHECK-FIXES: j(/*a=*/1, X(1), X(1));
   j(/*a=*/1, X(1), X(1));
 
   j(X(1), 1, X(1));