[PATCH] D89918: Fix issue: clang-format result is not consistent if "// clang-format off" is followed by macro definition.

2020-10-21 Thread Yubo Xie via Phabricator via cfe-commits 
xyb created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
xyb requested review of this revision.

For example, the above code:

  void main()
  {
  // clang-format off
  #define Sum(x, y) ((x) + (y))
  Sum(1, 2);
  #undef Sum
  // clang-format on
  }

If we run clang-format we will get the following result:

  void main()
  {
  // clang-format off
  #define Sum(x, y) ((x) + (y))
  Sum(1, 2);
  #undef Sum
  // clang-format on
  }

But if we run clang-format again, we will get another result:

  void main()
  {
  // clang-format off
  #define Sum(x, y) ((x) + (y))
  Sum(1, 2);
  #undef Sum
  // clang-format on
  }

I think the expectation should be "no mater how many times clang-format runs, 
the result should be the same."

This patch tries to fix this issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89918

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -17206,6 +17206,41 @@
"}",
Style);
 }
+
+TEST_F(FormatTest, FormatTurnOffFollowedByMacro1) {
+  EXPECT_EQ("void main() {\n"
+"  // clang-format off\n"
+"  #define Sum(x, y) ((x) + (y))\n"
+"  Sum(1, 2);\n"
+"  #undef Sum\n"
+"  // clang-format on\n"
+"};",
+format("void main() {\n"
+   "  // clang-format off\n"
+   "  #define Sum(x, y) ((x) + (y))\n"
+   "  Sum(1, 2);\n"
+   "  #undef Sum\n"
+   "  // clang-format on\n"
+   "};"));
+}
+
+TEST_F(FormatTest, FormatTurnOffFollowedByMacro2) {
+  EXPECT_EQ("void main() {\n"
+"  // clang-format off\n"
+"  #define Sum(x, y) ((x) + (y))\n"
+"  Sum(1, 2);\n"
+"  #undef Sum\n"
+"  // clang-format on\n"
+"};",
+format("void main() {\n"
+   "// clang-format off\n"
+   "  #define Sum(x, y) ((x) + (y))\n"
+   "  Sum(1, 2);\n"
+   "  #undef Sum\n"
+   "  // clang-format on\n"
+   "};"));
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2277,12 +2277,15 @@
   // Align comments for preprocessor lines with the # in column 0 if
   // preprocessor lines are not indented. Otherwise, align with the next
   // line.
-  (*I)->Level =
-  (Style.IndentPPDirectives != FormatStyle::PPDIS_BeforeHash &&
-   (NextNonCommentLine->Type == LT_PreprocessorDirective ||
-NextNonCommentLine->Type == LT_ImportStatement))
-  ? 0
-  : NextNonCommentLine->Level;
+  if ((*I)->First->TokenText != "// clang-format off" &&
+  (*I)->First->TokenText != "/* clang-format off */") {
+(*I)->Level =
+(Style.IndentPPDirectives != FormatStyle::PPDIS_BeforeHash &&
+ (NextNonCommentLine->Type == LT_PreprocessorDirective ||
+  NextNonCommentLine->Type == LT_ImportStatement))
+? 0
+: NextNonCommentLine->Level;
+  }
 } else {
   NextNonCommentLine = (*I)->First->isNot(tok::r_brace) ? (*I) : nullptr;
 }


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -17206,6 +17206,41 @@
"}",
Style);
 }
+
+TEST_F(FormatTest, FormatTurnOffFollowedByMacro1) {
+  EXPECT_EQ("void main() {\n"
+"  // clang-format off\n"
+"  #define Sum(x, y) ((x) + (y))\n"
+"  Sum(1, 2);\n"
+"  #undef Sum\n"
+"  // clang-format on\n"
+"};",
+format("void main() {\n"
+   "  // clang-format off\n"
+   "  #define Sum(x, y) ((x) + (y))\n"
+   "  Sum(1, 2);\n"
+   "  #undef Sum\n"
+   "  // clang-format on\n"
+   "};"));
+}
+
+TEST_F(FormatTest, FormatTurnOffFollowedByMacro2) {
+  EXPECT_EQ("void main() {\n"
+"  // clang-format off\n"
+"  #define Sum(x, y) ((x) + (y))\n"
+"  Sum(1, 2);\n"
+"  #undef Sum\n"
+"  // clang-format on\n"
+"};",
+format("void main() {\n"
+   "// clang-format off\n"
+   "  #define Sum(x, y) ((x)

[PATCH] D67501: [clang-tidy] Fix relative path in header-filter.

2019-09-23 Thread Yubo Xie via Phabricator via cfe-commits 
xyb added a comment.

> The only workable suggestion that we could come up with was adding a separate 
> command-line option, `-normalized-header-filter`, that would normalize paths 
> before matching the regex. It can be used by people whose project layout does 
> not have complex symlink mazes.

`-normalized-header-filter`. I'd like this idea. Any objections?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67501



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

[PATCH] D67501: [clang-tidy] Fix relative path in header-filter.

2019-09-20 Thread Yubo Xie via Phabricator via cfe-commits 
xyb added a comment.

Yes, I need your help to submit the patch. I don't have the permission. Thanks.


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

https://reviews.llvm.org/D67501



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

[PATCH] D67501: [clang-tidy] Fix relative path in header-filter.

2019-09-20 Thread Yubo Xie via Phabricator via cfe-commits 
xyb updated this revision to Diff 220997.
xyb marked 2 inline comments as done.
xyb added a comment.

Updated


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

https://reviews.llvm.org/D67501

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/test/clang-tidy/Inputs/file-filter/subfolder_a/header_a.h
  clang-tools-extra/test/clang-tidy/Inputs/file-filter/subfolder_b/header_b.h
  clang-tools-extra/test/clang-tidy/Inputs/file-filter/subfolder_c/header_c.h
  clang-tools-extra/test/clang-tidy/file-filter.cpp

Index: clang-tools-extra/test/clang-tidy/file-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/file-filter.cpp
+++ clang-tools-extra/test/clang-tidy/file-filter.cpp
@@ -9,6 +9,12 @@
 //   file-filter\header*.h due to code order between '/' and '\\'.
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK4 %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers -quiet %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK4-QUIET %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_a' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK5 %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_a' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK5-QUIET %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_b' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK6 %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_b' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK6-QUIET %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_c' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK7 %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_c' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK7-QUIET %s
 
 #include "header1.h"
 // CHECK-NOT: warning:
@@ -19,6 +25,12 @@
 // CHECK3-QUIET-NOT: warning:
 // CHECK4: header1.h:1:12: warning: single-argument constructors
 // CHECK4-QUIET: header1.h:1:12: warning: single-argument constructors
+// CHECK5-NOT: warning:
+// CHECK5-QUIET-NOT: warning:
+// CHECK6-NOT: warning:
+// CHECK6-QUIET-NOT: warning:
+// CHECK7-NOT: warning:
+// CHECK7-QUIET-NOT: warning:
 
 #include "header2.h"
 // CHECK-NOT: warning:
@@ -29,6 +41,44 @@
 // CHECK3-QUIET: header2.h:1:12: warning: single-argument constructors
 // CHECK4: header2.h:1:12: warning: single-argument constructors
 // CHECK4-QUIET: header2.h:1:12: warning: single-argument constructors
+// CHECK5-NOT: warning:
+// CHECK5-QUIET-NOT: warning:
+// CHECK6-NOT: warning:
+// CHECK6-QUIET-NOT: warning:
+// CHECK7-NOT: warning:
+// CHECK7-QUIET-NOT: warning:
+
+#include "subfolder_a/header_a.h"
+// CHECK-NOT: warning:
+// CHECK-QUIET-NOT: warning:
+// CHECK2: header_b.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK2-QUIET: header_b.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK3-NOT: warning:
+// CHECK3-QUIET-NOT: warning:
+// CHECK4: header_b.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK4-QUIET: header_b.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK5: header_a.h:3:12: warning: single-argument constructors must be marked explicit
+// CHECK5-QUIET: header_a.h:3:12: warning: single-argument constructors must be marked explicit
+// CHECK6: header_b.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK6-QUIET: header_b.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK7-NOT: warning:
+// CHECK7-QUIET-NOT: warning:
+
+#include "subfolder_c/header_c.h"
+// CHECK-NOT: warning:
+// CHECK-QUIET-NOT: warning:
+// CHECK2: header_c.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK2-QUIET: header_c.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK3-NOT: warning:
+// CHECK3-QUIET-NOT: warning:
+// CHECK4: header_c.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK4-QUIET: header_c.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK5-NOT: warning:
+// CHECK5-QUIET-NOT: warning:
+// CHECK6-NOT: warning:
+//

[PATCH] D67501: [clang-tidy] Fix relative path in header-filter.

2019-09-12 Thread Yubo Xie via Phabricator via cfe-commits 
xyb created this revision.
xyb added a reviewer: alexfh.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.

Clang-tidy supports output diagnostics from header files if user
specifies --header-filter. But it can't handle relative path well.
For example, the folder structure of a project is:

  // a.h is in /src/a/a.h
  #include "../b/b.h"
  
  // b.h is in /src/b/b.h
  ...
  
  // c.cpp is in /src/c.cpp
  #include "a/a.h"

Now, we set --header-filter as --header-filter=/a/. That means we only
want to check header files under /src/a/ path, and ignore header files
uder /src/b/ path, but in current implementation, clang-tidy will check
/src/b/b.h also, because the name of b.h used in clang-tidy is
/src/a/../b/b.h.

This change tries to fix this issue.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D67501

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/test/clang-tidy/Inputs/file-filter/subfolder_a/header.h
  clang-tools-extra/test/clang-tidy/Inputs/file-filter/subfolder_b/header.h
  clang-tools-extra/test/clang-tidy/Inputs/file-filter/subfolder_c/header.h
  clang-tools-extra/test/clang-tidy/file-filter.cpp

Index: clang-tools-extra/test/clang-tidy/file-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/file-filter.cpp
+++ clang-tools-extra/test/clang-tidy/file-filter.cpp
@@ -9,6 +9,12 @@
 //   file-filter\header*.h due to code order between '/' and '\\'.
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK4 %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers -quiet %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK4-QUIET %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_a' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK5 %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_a' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK5-QUIET %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_b' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK6 %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_b' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK6-QUIET %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_c' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK7 %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_c' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK7-QUIET %s
 
 #include "header1.h"
 // CHECK-NOT: warning:
@@ -19,6 +25,12 @@
 // CHECK3-QUIET-NOT: warning:
 // CHECK4: header1.h:1:12: warning: single-argument constructors
 // CHECK4-QUIET: header1.h:1:12: warning: single-argument constructors
+// CHECK5-NOT: warning:
+// CHECK5-QUIET-NOT: warning:
+// CHECK6-NOT: warning:
+// CHECK6-QUIET-NOT: warning:
+// CHECK7-NOT: warning:
+// CHECK7-QUIET-NOT: warning:
 
 #include "header2.h"
 // CHECK-NOT: warning:
@@ -29,6 +41,44 @@
 // CHECK3-QUIET: header2.h:1:12: warning: single-argument constructors
 // CHECK4: header2.h:1:12: warning: single-argument constructors
 // CHECK4-QUIET: header2.h:1:12: warning: single-argument constructors
+// CHECK5-NOT: warning:
+// CHECK5-QUIET-NOT: warning:
+// CHECK6-NOT: warning:
+// CHECK6-QUIET-NOT: warning:
+// CHECK7-NOT: warning:
+// CHECK7-QUIET-NOT: warning:
+
+#include "subfolder_a/header.h"
+// CHECK-NOT: warning:
+// CHECK-QUIET-NOT: warning:
+// CHECK2: header.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK2-QUIET: header.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK3-NOT: warning:
+// CHECK3-QUIET-NOT: warning:
+// CHECK4: header.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK4-QUIET: header.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK5: header.h:3:12: warning: single-argument constructors must be marked explicit
+// CHECK5-QUIET: header.h:3:12: warning: single-argument constructors must be marked explicit
+// CHECK6: header.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK6-QUIET: header.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK7-NOT: warning:
+//

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

2019-09-05 Thread Yubo Xie via Phabricator via cfe-commits 
xyb updated this revision to Diff 218893.
xyb added a comment.

Rebase patch with current HEAD.


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

https://reviews.llvm.org/D67084

Files:
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp


Index: clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
===
--- clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
+++ clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
@@ -69,18 +69,29 @@
   // 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]
@@ -98,14 +109,22 @@
   // 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));
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -230,9 +230,11 @@
 // 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(Arg))
+Arg = cast(Arg)->getSubExpr();
   if (Arg->getExprLoc().isMacroID())
 return false;
-  Arg = Arg->IgnoreImpCasts();
   return (CommentBoolLiterals && isa(Arg)) ||
  (CommentIntegerLiterals && isa(Arg)) ||
  (CommentFloatLiterals && isa(Arg)) ||


Index: clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
===
--- clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
+++ clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
@@ -69,18 +69,29 @@
   // 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]
@@ -98,14 +109,22 @@
   // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for literal argument 'dabc'

[PATCH] D67056: Add a bugprone-argument-comment option: IgnoreSingleArgument.

2019-09-05 Thread Yubo Xie via Phabricator via cfe-commits 
xyb updated this revision to Diff 218892.
xyb added a comment.

Update patch.


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

https://reviews.llvm.org/D67056

Files:
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
  clang-tools-extra/docs/clang-tidy/checks/bugprone-argument-comment.rst
  
clang-tools-extra/test/clang-tidy/bugprone-argument-comment-ignore-single-argument.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-argument-comment-ignore-single-argument.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-argument-comment-ignore-single-argument.cpp
@@ -0,0 +1,97 @@
+// RUN: %check_clang_tidy %s bugprone-argument-comment %t -- \
+// RUN:   -config="{CheckOptions: [{key: bugprone-argument-comment.IgnoreSingleArgument, value: 1}, {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);
+
+  a.foo(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);
+
+  a.foo(1.0f);
+
+  a.foo(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");
+
+  a.fooW(L"Hello World");
+
+  a.fooPtr(nullptr);
+
+  a.foo(402.0_km);
+
+  a.foo('A');
+
+  g(FOO);
+
+  h(1.0f);
+
+  i(__FILE__);
+
+  g((1));
+}
+
+void f(bool _with_underscores_);
+void ignores_underscores() {
+  f(false);
+
+  f(true);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-argument-comment.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/bugprone-argument-comment.rst
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-argument-comment.rst
@@ -28,6 +28,9 @@
underscores and case when comparing names -- otherwise they are taken into
account.
 
+.. option:: IgnoreSingleArgument
+   When true, the check will ignore the single argument.
+
 .. option:: CommentBoolLiterals
 
When true, the check will add argument comments in the format
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
@@ -41,6 +41,7 @@
 
 private:
   const unsigned StrictMode : 1;
+  const unsigned IgnoreSingleArgument : 1;
   const unsigned CommentBoolLiterals : 1;
   const unsigned CommentIntegerLiterals : 1;
   const unsigned CommentFloatLiterals : 1;
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -24,6 +24,7 @@
ClangTidyContext *Context)
 : ClangTidyCheck(Name,

[PATCH] D67056: Add a bugprone-argument-comment option: IgnoreSingleArgument.

2019-09-04 Thread Yubo Xie via Phabricator via cfe-commits 
xyb marked 2 inline comments as done.
xyb added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp:28
+  IgnoreSingleArgument(
+  Options.getLocalOrGlobal("IgnoreSingleArgument", 0) != 0),
   CommentBoolLiterals(Options.getLocalOrGlobal("CommentBoolLiterals", 0) !=

alexfh wrote:
> xyb wrote:
> > alexfh wrote:
> > > Why is it a global option? Are there other checks that would need the 
> > > same option? The one below also needs to be check-local.
> > Sorry, I'm afraid I didn't get your point. Could you please explain more? 
> > This setting just follows the same pattern used for other settings. All 
> > other settings use "Options.getLocalOrGlobal(...)", I'm not sure why this 
> > setting need be different. Or do you mean we should change other settings 
> > ("StrictMode", "CommentBoolLiterals", "CommentIntegerLiterals", ...) to 
> > local options also?
> Options are stored as a string -> string map. The key in this map is either 
> the option name prepended with the check name (for check-local options) or 
> just the option name (this way an option can be read by all checks). There 
> are two ways to read options: OptionsView::get reads just the local name, 
> OptionsView::getLocalOrGlobal tries the check-local name and then falls back 
> to reading the option using its global name.
> 
> In this particular check only the StrictMode option makes sense to be global 
> (a few other checks define an option with the same name and set of values, 
> and it may make sense to configure a global default value). Other options are 
> specific to this check and should be local.
Thanks for your explanation! Updated. 


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

https://reviews.llvm.org/D67056



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

[PATCH] D67056: Add a bugprone-argument-comment option: IgnoreSingleArgument.

2019-09-04 Thread Yubo Xie via Phabricator via cfe-commits 
xyb updated this revision to Diff 218741.

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

https://reviews.llvm.org/D67056

Files:
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
  clang-tools-extra/docs/clang-tidy/checks/bugprone-argument-comment.rst
  
clang-tools-extra/test/clang-tidy/bugprone-argument-comment-ignore-single-argument.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-argument-comment-ignore-single-argument.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-argument-comment-ignore-single-argument.cpp
@@ -0,0 +1,97 @@
+// RUN: %check_clang_tidy %s bugprone-argument-comment %t -- \
+// RUN:   -config="{CheckOptions: [{Key: IgnoreSingleArgument, value: 1}, {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);
+
+  a.foo(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);
+
+  a.foo(1.0f);
+
+  a.foo(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");
+
+  a.fooW(L"Hello World");
+
+  a.fooPtr(nullptr);
+
+  a.foo(402.0_km);
+
+  a.foo('A');
+
+  g(FOO);
+  
+  h(1.0f);
+
+  i(__FILE__);
+
+  g((1));
+}
+
+void f(bool _with_underscores_);
+void ignores_underscores() {
+  f(false);
+
+  f(true);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-argument-comment.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/bugprone-argument-comment.rst
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-argument-comment.rst
@@ -28,6 +28,9 @@
underscores and case when comparing names -- otherwise they are taken into
account.
 
+.. option:: IgnoreSingleArgument
+   When true, the check will ignore the single argument. 
+
 .. option:: CommentBoolLiterals
 
When true, the check will add argument comments in the format
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
@@ -41,6 +41,7 @@
 
 private:
   const unsigned StrictMode : 1;
+  const unsigned IgnoreSingleArgument : 1;
   const unsigned CommentBoolLiterals : 1;
   const unsigned CommentIntegerLiterals : 1;
   const unsigned CommentFloatLiterals : 1;
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -24,6 +24,7 @@
ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),

[PATCH] D67056: Add a bugprone-argument-comment option: IgnoreSingleArgument.

2019-09-04 Thread Yubo Xie via Phabricator via cfe-commits 
xyb added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp:28
+  IgnoreSingleArgument(
+  Options.getLocalOrGlobal("IgnoreSingleArgument", 0) != 0),
   CommentBoolLiterals(Options.getLocalOrGlobal("CommentBoolLiterals", 0) !=

alexfh wrote:
> Why is it a global option? Are there other checks that would need the same 
> option? The one below also needs to be check-local.
Sorry, I'm afraid I didn't get your point. Could you please explain more? This 
setting just follows the same pattern used for other settings. All other 
settings use "Options.getLocalOrGlobal(...)", I'm not sure why this setting 
need be different. Or do you mean we should change other settings 
("StrictMode", "CommentBoolLiterals", "CommentIntegerLiterals", ...) to local 
options also?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D67056



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

[PATCH] D67056: Add a bugprone-argument-comment option: IgnoreSingleArgument.

2019-09-04 Thread Yubo Xie via Phabricator via cfe-commits 
xyb added a comment.

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


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D67056



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

[PATCH] D67080: [clang-tidy] Fix bugprone-argument-comment bug if there are marcos.

2019-09-04 Thread Yubo Xie via Phabricator via cfe-commits 
xyb added a comment.

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


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D67080



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

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

2019-09-04 Thread Yubo Xie via Phabricator via cfe-commits 
xyb added a comment.

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


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

https://reviews.llvm.org/D67084



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

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

2019-09-04 Thread Yubo Xie via Phabricator via cfe-commits 
xyb updated this revision to Diff 218683.
xyb added a comment.

Update with full diff.


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

https://reviews.llvm.org/D67084

Files:
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp


Index: clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
===
--- clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
+++ clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
@@ -67,18 +67,29 @@
   // 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]
@@ -96,14 +107,22 @@
   // 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__);
 
   // FIXME Would like the below to add argument comments.
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -230,9 +230,11 @@
 // 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(Arg))
+Arg = cast(Arg)->getSubExpr();
   if (Arg->getExprLoc().isMacroID())
 return false;
-  Arg = Arg->IgnoreImpCasts();
   return (CommentBoolLiterals && isa(Arg)) ||
  (CommentIntegerLiterals && isa(Arg)) ||
  (CommentFloatLiterals && isa(Arg)) ||


Index: clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
===
--- clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
+++ clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
@@ -67,18 +67,29 @@
   // 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]
@@ -96,14 +107,22 @@
   // CHECK-MESSAGES: [[@LINE-1]]:9: warning: argument comment missing for

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

2019-09-04 Thread Yubo Xie via Phabricator via cfe-commits 
xyb added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp:240
+  Arg = Arg->IgnoreImpCasts();
+  if (isa(Arg))
+  Arg = cast(Arg)->getSubExpr();

aaron.ballman wrote:
> 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?
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? 


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D67084



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

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

2019-09-02 Thread Yubo Xie via Phabricator via cfe-commits 
xyb created this revision.
xyb added a reviewer: alexfh.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

For example:

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

should be fixed as:

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

This change tries to fix this issue.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D67084

Files:
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp


Index: clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
===
--- clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
+++ clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
@@ -69,18 +69,29 @@
   // 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]
@@ -98,14 +109,22 @@
   // 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));
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -236,9 +236,11 @@
 // 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(Arg))
+  Arg = cast(Arg)->getSubExpr();
   if (Arg->getExprLoc().isMacroID())
 return false;
-  Arg = Arg->IgnoreImpCasts();
   return (CommentBoolLiterals && isa(Arg)) ||
  (CommentIntegerLiterals && isa(Arg)) ||
  (CommentFloatLiterals && isa(Arg)) ||


Index: clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
===
--- clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
+++ clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
@@ -69,18 +69,29 @@
   // 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]
@@

[PATCH] D67080: Fix bugprone-argument-comment bug if there are marcos.

2019-09-02 Thread Yubo Xie via Phabricator via cfe-commits 
xyb updated this revision to Diff 218372.

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

https://reviews.llvm.org/D67080

Files:
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp


Index: clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
===
--- clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
+++ clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
@@ -15,10 +15,12 @@
 };
 
 #define FOO 1
+#define X(x) (x)
 
 void g(int a);
 void h(double b);
 void i(const char *c);
+void j(int a, int b, int c);
 
 double operator"" _km(long double);
 
@@ -106,6 +108,39 @@
   // 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));
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: argument comment missing for 
literal argument 'b' [bugprone-argument-comment]
+  // CHECK-FIXES: j(X(1), /*b=*/1, X(1));
+  j(X(1), /*b=*/1, X(1));
+
+  j(X(1), X(1), 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: argument comment missing for 
literal argument 'c' [bugprone-argument-comment]
+  // CHECK-FIXES: j(X(1), X(1), /*c=*/1);
+  j(X(1), X(1), /*c=*/1);
+
+  j(X(1), 1, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: argument comment missing for 
literal argument 'b' [bugprone-argument-comment]
+  // CHECK-MESSAGES: [[@LINE-2]]:14: warning: argument comment missing for 
literal argument 'c' [bugprone-argument-comment]
+  // CHECK-FIXES: j(X(1), /*b=*/1, /*c=*/1);
+  j(X(1), /*b=*/1, /*c=*/1);
+
+  j(1, X(1), 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: argument comment missing for 
literal argument 'a' [bugprone-argument-comment]
+  // CHECK-MESSAGES: [[@LINE-2]]:14: warning: argument comment missing for 
literal argument 'c' [bugprone-argument-comment]
+  // CHECK-FIXES: j(/*a=*/1, X(1), /*c=*/1);
+  j(/*a=*/1, X(1), /*c=*/1);
+
+  j(1, 1, X(1));
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: argument comment missing for 
literal argument 'a' [bugprone-argument-comment]
+  // CHECK-MESSAGES: [[@LINE-2]]:8: warning: argument comment missing for 
literal argument 'b' [bugprone-argument-comment]
+  // CHECK-FIXES: j(/*a=*/1, /*b=*/1, X(1));
+  j(/*a=*/1, /*b=*/1, X(1));
+
   // FIXME Would like the below to add argument comments.
   g((1));
   // FIXME But we should not add argument comments here.
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -322,8 +322,8 @@
   Comments = getCommentsInRange(Ctx, BeforeArgument);
 } else {
   // Fall back to parsing back from the start of the argument.
-  CharSourceRange ArgsRange = MakeFileCharRange(
-  Args[I]->getBeginLoc(), Args[NumArgs - 1]->getEndLoc());
+  CharSourceRange ArgsRange =
+  MakeFileCharRange(Args[I]->getBeginLoc(), Args[I]->getEndLoc());
   Comments = getCommentsBeforeLoc(Ctx, ArgsRange.getBegin());
 }
 


Index: clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
===
--- clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
+++ clang-tools-extra/test/clang-tidy/bugprone-argument-comment-literals.cpp
@@ -15,10 +15,12 @@
 };
 
 #define FOO 1
+#define X(x) (x)
 
 void g(int a);
 void h(double b);
 void i(const char *c);
+void j(int a, int b, int c);
 
 double operator"" _km(long double);
 
@@ -106,6 +108,39 @@
   // 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));
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: argument comment missing for literal argument 'b' [bugprone-argument-comment]
+  // CHECK-FIXES: j(X(1), /*b=*/1, X(1));
+  j(X(1), /*b=*/1, X(1));
+
+  j(X(1), X(1), 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:17: warning: argument comment missing for literal argument 'c' [bugprone-argument-comment]
+  // CHECK-FIXES: j(X(1), X(1), /*c=*/1);
+  j(X(1), X(1), /*c=*/1);
+
+  j(X(1), 1, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: argument comment missing for literal argument 'b' [bugprone-argument-comment]
+  // CHECK-MESSAGES: [[@LINE-2]]:14: warning: argument comment missing for literal argument 'c' [bugprone-argument-comment]
+  // CHECK-FIXES: j(X(1), /*b=*/1, /*c=*/1);
+  j(X(1), /*b=*/1, /*c=*/1);
+
+  j(1,

[PATCH] D67080: Fix bugprone-argument-comment bug if there are marcos.

2019-09-02 Thread Yubo Xie via Phabricator via cfe-commits 
xyb created this revision.
xyb added a reviewer: alexfh.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fix bugprone-argument-comment bug if there are marcos.

For example:

  #define X(x) (x)
  void j(int a, int b, int c);
  j(X(1), /*b=*/1, X(1));

clang-tidy can't recognize comment "/*b=*/". It suggests fix like this:

  j(X(1), /*b=*//*b=*/1, X(1));

This change tries to fix this issue.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D67080

Files:





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

[PATCH] D67056: Add a bugprone-argument-comment option: IgnoreSingleArgument.

2019-09-01 Thread Yubo Xie via Phabricator via cfe-commits 
xyb created this revision.
xyb added a reviewer: alexfh.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Add bugprone-argument-comment option: IgnoreSingleArgument.
When true, the check will ignore the single argument.

Sometimes, it's not necessary to add comment to single argument.
For example:

> std::string name("Yubo Xie");
>  pScreen->SetWidth(1920);
>  pScreen->SetHeight(1080);

This option can ignore such single argument in bugprone-argument-comment check.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D67056

Files:
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
  clang-tools-extra/docs/clang-tidy/checks/bugprone-argument-comment.rst
  
clang-tools-extra/test/clang-tidy/bugprone-argument-comment-ignore-single-argument.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-argument-comment-ignore-single-argument.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-argument-comment-ignore-single-argument.cpp
@@ -0,0 +1,97 @@
+// RUN: %check_clang_tidy %s bugprone-argument-comment %t -- \
+// RUN:   -config="{CheckOptions: [{key: IgnoreSingleArgument, value: 1}, {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);
+
+  a.foo(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);
+
+  a.foo(1.0f);
+
+  a.foo(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");
+
+  a.fooW(L"Hello World");
+
+  a.fooPtr(nullptr);
+
+  a.foo(402.0_km);
+
+  a.foo('A');
+
+  g(FOO);
+
+  h(1.0f);
+
+  i(__FILE__);
+
+  g((1));
+}
+
+void f(bool _with_underscores_);
+void ignores_underscores() {
+  f(false);
+
+  f(true);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-argument-comment.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/bugprone-argument-comment.rst
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-argument-comment.rst
@@ -28,6 +28,9 @@
underscores and case when comparing names -- otherwise they are taken into
account.
 
+.. option:: IgnoreSingleArgument
+   When true, the check will ignore the single argument.
+
 .. option:: CommentBoolLiterals
 
When true, the check will add argument comments in the format
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
@@ -41,6 +41,7 @@
 
 private:
   const unsigned StrictMode : 1;
+  const unsigned IgnoreSingleArgument : 1;
   const unsigned CommentBoolLiterals : 1;
   const unsigned CommentIntegerLiterals : 1;
   const unsigned CommentFloatLiterals : 1;
Index: