[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa added a comment.

In https://reviews.llvm.org/D40671#954661, @alexfh wrote:

> In https://reviews.llvm.org/D40671#953888, @aaron.ballman wrote:
>
> > FWIW, I think we should do something about unknown check names in NOLINT 
> > comments, but that can be done as a follow-up patch. If we're ignoring the 
> > comment, we might want to diagnose that fact so users have an idea what's 
> > going on.
>
>
> IIUC, cpplint can output a diagnostic about unknown categories inside NOLINT 
> and about NOLINT directives that happen on lines where no warning is emitted. 
> Both would be useful in clang-tidy, IMO.


I agree with your statements and I think there should be the following 
diagnostics about NOLINT usage:

- as you described, using of NOLINT with unknown check names;
- using of NOLINT for the line, on which there is no diagnostics (at all with 
NOLINT and for the swpecified diagnostics); this should help to detect dangling 
NOLINT comments, that have no meaning anymore.

Moreover, there should be a way to turn on/off these diagnostics, so possibily 
they should be a separate checks. What do you think? Is there a way for a check 
to collect the emitted diagnostics?


https://reviews.llvm.org/D40671



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

[PATCH] D39622: Fix type name generation in DWARF for template instantiations with enum types and template specializations

[Anton via Phabricator via cfe-commits]

xgsa added a comment.

In https://reviews.llvm.org/D39622#954585, @probinson wrote:

> Philosophically, mangled names and DWARF information serve different 
> purposes, and I don't think you will find one true solution where both of 
> them can yield the same name that everyone will be happy with.  Mangled names 
> exist to provide unique and reproducible identifiers for the "same" entity 
> across compilation units.  They are carefully specified (for example) to 
> allow a linker to associate a reference in one object file to a definition in 
> a different object file, and be guaranteed that the association is correct.  
> A demangled name is a necessarily context-free translation of the mangled 
> name into something that has a closer relationship to how a human would think 
> of or write the name of the thing, but isn't necessarily the only way to 
> write the name of the thing.
>
> DWARF names are (deliberately not carefully specified) strings that ought to 
> bear some relationship to how source code would name the thing, but you 
> probably don't want to attach semantic significance to those names.  This is 
> rather emphatically true for names containing template parameters.  Typedefs 
> (and their recent offspring, 'using' aliases) are your sworn enemy here.  
> Enums, as you have found, are also a problem.
>
> Basically, the type of an entity does not have a unique name, and trying to 
> coerce different representations of the type into having the same unique name 
> is a losing battle.


Thank you for clarification, Paul! Nevertheless, I suppose, showing actual type 
of a dynamic variable is very important for the projects, where RTTI is used. 
Moreover, it works properly in gcc+gdb pair, so I am extremely interested in 
fixing it in clang+lldb.

I understand that the suggested solution possibly does not cover all the cases, 
but it improves the situation and actually covers all the cases found by me (I 
have just rechecked -- typedefs/usings seems to work fine when displaying the 
real type of variable). If more cases are found in future, they could be fixed 
similarly too. Moreover, the debuggers already rely on the fact that the type 
name looks the same in RTTI and DWARF, and I suppose they have no choice, 
because there is no other source of information for them (or am I missing 
something?). Another advantage of this solution is that it doesn't require any 
format extension and will probably work out of the box in gdb and other 
debuggers. Moreover, I have just rechecked, gcc generates exactly the same type 
names in DWARF for examples in the description.

On the other hand, I understand the idea you have described, but I am not sure 
how to implement this lookup in another way. I suppose, we cannot extend RTTI 
with the debug type name (is it correct?). Thus, the only way I see is to add 
additional information about the mangled type name into DWARF. It could be 
either a separate section (like apple_types) or a special node for 
TAG_structure_type/TAG_class_type, which should be indexed into map for fast 
lookup. Anyway, this will be an extension to DWARF and will require special 
support in a debugger. Furthermore, such solution will be much complicated 
(still I don't mind working on it).

So what do you think? Is the suggested solution not full or not acceptable? Do 
you have other ideas how this feature should be implemented?

P.S. Should this question be raised in mailing list? And if yes, actually, in 
which ones (clang or lldb?), because it seems related to both clang and lldb?


https://reviews.llvm.org/D39622



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

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa created this revision.
xgsa added reviewers: aaron.ballman, alexfh.
xgsa added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun.

As discussed in the previous review [1], diagnostics about incorrect usage of 
NOLINT comment was added, i.e..:

- usage of NOLINT with unknown check name;
- usage of NOLINT for line, where there is no diagnostics;
- usage of NOLINT without closing parenthesis.

I have covered the implementation with tests, but I haven't updated the 
documentation yet, because I'd like to approve the implemented approach in 
general. If nobody insists, I'd prefer updating the documentation in follow-up 
patches, because this will make the patch even bigger and the review longer.

[1] - https://reviews.llvm.org/D40671


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41326

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  test/clang-tidy/nolint-usage.cpp
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -24,6 +24,18 @@
 class C5 { C5(int i); };
 
 
+void f() {
+  int i;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
+  // NOLINTNEXTLINE
+  int j;
+  // NOLINTNEXTLINE(clang-diagnostic-unused-variable)
+  int k;
+  // NOLINTNEXTLINE(clang-diagnostic-literal-conversion)
+  int l;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
+}
+
 // NOLINTNEXTLINE
 
 class D { D(int i); };
@@ -44,6 +56,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 10 warnings (10 NOLINT)
 
-// RUN: %check_clang_tidy %s google-explicit-constructor %t --
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable %t -- -extra-arg=-Wunused-variable --
Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -30,6 +30,9 @@
   int i;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
   int j; // NOLINT
+  int k; // NOLINT(clang-diagnostic-unused-variable)
+  int l; // NOLINT(clang-diagnostic-literal-conversion)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
 }
 
 #define MACRO(X) class X { X(int i); };
@@ -46,4 +49,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
+// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)
Index: test/clang-tidy/nolint-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/nolint-usage.cpp
@@ -0,0 +1,171 @@
+// RUN: %check_clang_tidy %s nolint-usage,google-explicit-constructor,misc-unused-parameters %t --
+
+// Case: NO_LINT on line with an error.
+class A1 { A1(int i); }; // NOLINT
+
+// Case: NO_LINT on line without errors.
+class A2 { explicit A2(int i); }; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for the specific check on line with an error on it.
+class A3 { A3(int i); }; // NOLINT(google-explicit-constructor)
+
+// Case: NO_LINT for the specific check on line with an error on another check.
+class A4 { A4(int i); }; // NOLINT(misc-unused-parameters)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: there is no diagnostics for the 'misc-unused-parameters' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for the specific check on line without errors.
+class A5 { explicit A5(int i); }; // NOLINT(google-explicit-constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there is no diagnostics for the 'google-explicit-constructor' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for unknown check on line with an error on some check.
+class A6 { A6(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: unknown check name 'unknown-check' is specified for NOLINT comment
+
+// Case: NO_LINT for unknown check on line without errors.
+class A7 { explicit A7(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: unknown check name 'unknown-check' is specified for NOLINT comment
+
+// Case: NO_LINT with not closed parenthesis without check names.
+class A8 { A8(int i); }; // NOLINT(
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: NOLINT comment has an open

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 127260.
xgsa added a comment.

A few minor coding style fixes.


https://reviews.llvm.org/D41326

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  test/clang-tidy/nolint-usage.cpp
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -24,6 +24,18 @@
 class C5 { C5(int i); };
 
 
+void f() {
+  int i;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
+  // NOLINTNEXTLINE
+  int j;
+  // NOLINTNEXTLINE(clang-diagnostic-unused-variable)
+  int k;
+  // NOLINTNEXTLINE(clang-diagnostic-literal-conversion)
+  int l;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
+}
+
 // NOLINTNEXTLINE
 
 class D { D(int i); };
@@ -44,6 +56,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 10 warnings (10 NOLINT)
 
-// RUN: %check_clang_tidy %s google-explicit-constructor %t --
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable %t -- -extra-arg=-Wunused-variable --
Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -30,6 +30,9 @@
   int i;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
   int j; // NOLINT
+  int k; // NOLINT(clang-diagnostic-unused-variable)
+  int l; // NOLINT(clang-diagnostic-literal-conversion)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
 }
 
 #define MACRO(X) class X { X(int i); };
@@ -46,4 +49,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
+// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)
Index: test/clang-tidy/nolint-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/nolint-usage.cpp
@@ -0,0 +1,171 @@
+// RUN: %check_clang_tidy %s nolint-usage,google-explicit-constructor,misc-unused-parameters %t --
+
+// Case: NO_LINT on line with an error.
+class A1 { A1(int i); }; // NOLINT
+
+// Case: NO_LINT on line without errors.
+class A2 { explicit A2(int i); }; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for the specific check on line with an error on it.
+class A3 { A3(int i); }; // NOLINT(google-explicit-constructor)
+
+// Case: NO_LINT for the specific check on line with an error on another check.
+class A4 { A4(int i); }; // NOLINT(misc-unused-parameters)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: there is no diagnostics for the 'misc-unused-parameters' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for the specific check on line without errors.
+class A5 { explicit A5(int i); }; // NOLINT(google-explicit-constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there is no diagnostics for the 'google-explicit-constructor' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for unknown check on line with an error on some check.
+class A6 { A6(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: unknown check name 'unknown-check' is specified for NOLINT comment
+
+// Case: NO_LINT for unknown check on line without errors.
+class A7 { explicit A7(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: unknown check name 'unknown-check' is specified for NOLINT comment
+
+// Case: NO_LINT with not closed parenthesis without check names.
+class A8 { A8(int i); }; // NOLINT(
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: NOLINT comment has an opening parenthesis and no closing one, so all the diagnostics will be silenced for the line
+
+// Case: NO_LINT with not closed parenthesis with check names.
+class A9 { A9(int i); }; // NOLINT(unknown-check
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: NOLINT comment has an opening parenthesis and no closing one, so all the diagnostics will be silenced for the line
+
+// Case: NO_LINT with clang diagnostics
+int f() {
+  int i = 0; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+  int j = 0; // NOLINT(clang-diagnostic-unused-variable)
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there is no diagnostics for the 'clang-diagnostic

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

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



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:339
 std::unique_ptr OptionsProvider)
 : DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
+  Profile(nullptr),

Eugene.Zelenko wrote:
> Please use default members initialization for DiagEngine and Profile.
Actually, it is not necessary, as unique_ptr is initialized with nullptr by 
default. I have just removed initializing them here.


https://reviews.llvm.org/D41326



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

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 127267.
xgsa added a comment.

Review comments were applied.


https://reviews.llvm.org/D41326

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  test/clang-tidy/nolint-usage.cpp
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -24,6 +24,18 @@
 class C5 { C5(int i); };
 
 
+void f() {
+  int i;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
+  // NOLINTNEXTLINE
+  int j;
+  // NOLINTNEXTLINE(clang-diagnostic-unused-variable)
+  int k;
+  // NOLINTNEXTLINE(clang-diagnostic-literal-conversion)
+  int l;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
+}
+
 // NOLINTNEXTLINE
 
 class D { D(int i); };
@@ -44,6 +56,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 10 warnings (10 NOLINT)
 
-// RUN: %check_clang_tidy %s google-explicit-constructor %t --
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable %t -- -extra-arg=-Wunused-variable --
Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -30,6 +30,9 @@
   int i;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
   int j; // NOLINT
+  int k; // NOLINT(clang-diagnostic-unused-variable)
+  int l; // NOLINT(clang-diagnostic-literal-conversion)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
 }
 
 #define MACRO(X) class X { X(int i); };
@@ -46,4 +49,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
+// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)
Index: test/clang-tidy/nolint-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/nolint-usage.cpp
@@ -0,0 +1,171 @@
+// RUN: %check_clang_tidy %s nolint-usage,google-explicit-constructor,misc-unused-parameters %t --
+
+// Case: NO_LINT on line with an error.
+class A1 { A1(int i); }; // NOLINT
+
+// Case: NO_LINT on line without errors.
+class A2 { explicit A2(int i); }; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for the specific check on line with an error on it.
+class A3 { A3(int i); }; // NOLINT(google-explicit-constructor)
+
+// Case: NO_LINT for the specific check on line with an error on another check.
+class A4 { A4(int i); }; // NOLINT(misc-unused-parameters)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: there is no diagnostics for the 'misc-unused-parameters' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for the specific check on line without errors.
+class A5 { explicit A5(int i); }; // NOLINT(google-explicit-constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there is no diagnostics for the 'google-explicit-constructor' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for unknown check on line with an error on some check.
+class A6 { A6(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: unknown check name 'unknown-check' is specified for NOLINT comment
+
+// Case: NO_LINT for unknown check on line without errors.
+class A7 { explicit A7(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: unknown check name 'unknown-check' is specified for NOLINT comment
+
+// Case: NO_LINT with not closed parenthesis without check names.
+class A8 { A8(int i); }; // NOLINT(
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: NOLINT comment has an opening parenthesis and no closing one, so all the diagnostics will be silenced for the line
+
+// Case: NO_LINT with not closed parenthesis with check names.
+class A9 { A9(int i); }; // NOLINT(unknown-check
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: NOLINT comment has an opening parenthesis and no closing one, so all the diagnostics will be silenced for the line
+
+// Case: NO_LINT with clang diagnostics
+int f() {
+  int i = 0; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+  int j = 0; // NOLINT(clang-diagnostic-unused-variable)
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there is no diagnostics for the 'clang-diagnostic-u

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa added inline comments.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:339
 std::unique_ptr OptionsProvider)
 : DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
+  Profile(nullptr),

xgsa wrote:
> Eugene.Zelenko wrote:
> > Please use default members initialization for DiagEngine and Profile.
> Actually, it is not necessary, as unique_ptr is initialized with nullptr by 
> default. I have just removed initializing them here.
Sorry, you are right, just ignore my previous comment.


https://reviews.llvm.org/D41326



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

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 127268.
xgsa added a comment.

Review comments applied.


https://reviews.llvm.org/D41326

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  test/clang-tidy/nolint-usage.cpp
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -24,6 +24,18 @@
 class C5 { C5(int i); };
 
 
+void f() {
+  int i;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
+  // NOLINTNEXTLINE
+  int j;
+  // NOLINTNEXTLINE(clang-diagnostic-unused-variable)
+  int k;
+  // NOLINTNEXTLINE(clang-diagnostic-literal-conversion)
+  int l;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
+}
+
 // NOLINTNEXTLINE
 
 class D { D(int i); };
@@ -44,6 +56,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 10 warnings (10 NOLINT)
 
-// RUN: %check_clang_tidy %s google-explicit-constructor %t --
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable %t -- -extra-arg=-Wunused-variable --
Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -30,6 +30,9 @@
   int i;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
   int j; // NOLINT
+  int k; // NOLINT(clang-diagnostic-unused-variable)
+  int l; // NOLINT(clang-diagnostic-literal-conversion)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
 }
 
 #define MACRO(X) class X { X(int i); };
@@ -46,4 +49,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
+// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)
Index: test/clang-tidy/nolint-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/nolint-usage.cpp
@@ -0,0 +1,171 @@
+// RUN: %check_clang_tidy %s nolint-usage,google-explicit-constructor,misc-unused-parameters %t --
+
+// Case: NO_LINT on line with an error.
+class A1 { A1(int i); }; // NOLINT
+
+// Case: NO_LINT on line without errors.
+class A2 { explicit A2(int i); }; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for the specific check on line with an error on it.
+class A3 { A3(int i); }; // NOLINT(google-explicit-constructor)
+
+// Case: NO_LINT for the specific check on line with an error on another check.
+class A4 { A4(int i); }; // NOLINT(misc-unused-parameters)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: there is no diagnostics for the 'misc-unused-parameters' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for the specific check on line without errors.
+class A5 { explicit A5(int i); }; // NOLINT(google-explicit-constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there is no diagnostics for the 'google-explicit-constructor' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for unknown check on line with an error on some check.
+class A6 { A6(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: unknown check name 'unknown-check' is specified for NOLINT comment
+
+// Case: NO_LINT for unknown check on line without errors.
+class A7 { explicit A7(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: unknown check name 'unknown-check' is specified for NOLINT comment
+
+// Case: NO_LINT with not closed parenthesis without check names.
+class A8 { A8(int i); }; // NOLINT(
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: NOLINT comment has an opening parenthesis and no closing one, so all the diagnostics will be silenced for the line
+
+// Case: NO_LINT with not closed parenthesis with check names.
+class A9 { A9(int i); }; // NOLINT(unknown-check
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: NOLINT comment has an opening parenthesis and no closing one, so all the diagnostics will be silenced for the line
+
+// Case: NO_LINT with clang diagnostics
+int f() {
+  int i = 0; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+  int j = 0; // NOLINT(clang-diagnostic-unused-variable)
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there is no diagnostics for the 'clang-diagnostic-unused

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa added a comment.

In https://reviews.llvm.org/D41326#957749, @JVApen wrote:

> I'm missing some documentation to understand the corner cases. How does this 
> check behave with suppressed warnings for checks which ain't currently 
> checked. (Using -no-... on a code base or suppressing the warnings via the 
> pragmas)


The check just gathers NOLINT comments and compares them with a list of 
reported diagnostics for the current configuration. Thus, it will report 
diagnostics on the NOLINT comments, which are added for the checks, which are 
disabled in current configuration (e.g. with option -check=-some_check).

Note also that in spite NOLINT for clang warnings now works for clang-tidy, it 
doesn't work for clang itself, so I wouldn't recommended using something like 
"// NOLINT(clang-diagnostic-unused-variable)". As you mentioned, pragmas or 
-no-... options should be used instead. The check doesn't perform any 
processing of those things.


https://reviews.llvm.org/D41326



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

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa added a comment.

Ping.


https://reviews.llvm.org/D41326



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

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa marked 13 inline comments as done.
xgsa added a comment.

Aaron, thank you for your review and sorry for the coding convention mistakes 
-- I still cannot get used to the llvm coding convention, because it quite 
differs from the one I have been using in my projects.




Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:276
+
+  using NolintMap = std::unordered_map Is there a better LLVM ADT to use here?
This data structure provides the fast lookup by check name+line number and it's 
exactly what is necessary. What are the concerns about this data structure?



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:316
+  Context->diag(NolintCheckName, NolintLoc,
+  "unknown check name '%0' is specified for %1 comment")
+  << CheckName << CommentName;

aaron.ballman wrote:
> I'd reword this slightly to: "unknown check name '%0' specified in %1 
> directive"
I'd used the word "comment" instead of "directive" for consistency with 
documentation and the other messages. The rest is applied.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:424
+// This method is not const, because it fills cache on first demand.
+// It is possible to fill cache in constructor or make cache volatile
+// and make this method const, but they seem like worse solutions.

aaron.ballman wrote:
> Making the cache volatile will have no impact on this.
> 
> Any reason not to make the cache `mutable`, however? That's quite common for 
> implementation details.
Sorry, certainly, instead of "volatile" I meant "mutable".

Actually, using of "mutable" violates a constancy contract, as the field is get 
modified in a const method. Thus I'd tend to avoid using `mutable`, if 
possible, because e.g. in multi-threaded applications these fields require 
additional protection/synchronization. Moreover, I see that using of  `mutable` 
is not very spread in clang-tidy. Thus as, currently, `hasCheck` is called from 
the non-constant context, I'd prefer leaving it non-const instead of making 
cache `mutable`. Please, let me know if you insist on the `mutable` option.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:839-840
+case NolintCommentType::Nolint:
+  Message = "there is no diagnostics on this line, "
+"the NOLINT comment is redundant";
+  break;

aaron.ballman wrote:
> I don't think the user is going to care about the distinction between no 
> diagnostics being triggered and the expected diagnostic not being triggered. 
> Also, it's dangerous to claim the lint comment is redundant because it's 
> possible the user has NOLINT(foo, bar) and while foo is not triggered, bar 
> still is. The NOLINT comment itself isn't redundant, it's that the check 
> specified doesn't occur.
> 
> I would consolidate those scenarios into a single diagnostic: "expected 
> diagnostic '%0' not generated" and "expected diagnostic '%0' not generated 
> for the following line".
> 
> One concern I have with this functionality is: how should users silence a 
> lint diagnostic that's target sensitive? e.g., a diagnostic that triggers 
> based on the underlying type of size_t or the signedness of plain char. In 
> that case, the diagnostic may trigger for some targets but not others, but on 
> the targets where the diagnostic is not triggered, they now get a diagnostic 
> they cannot silence. There should be a way to silence the "bad NOLINT" 
> diagnostics.
> I don't think the user is going to care about the distinction between no 
> diagnostics being triggered and the expected diagnostic not being triggered. 
> Also, it's dangerous to claim the lint comment is redundant because it's 
> possible the user has NOLINT(foo, bar) and while foo is not triggered, bar 
> still is. The NOLINT comment itself isn't redundant, it's that the check 
> specified doesn't occur.
> 
> I would consolidate those scenarios into a single diagnostic: "expected 
> diagnostic '%0' not generated" and "expected diagnostic '%0' not generated 
> for the following line".

This branch of `if (NolintEntry.first.CheckName == 
NolintCommentsCollector::AnyCheck)` reports only about 
`NOLINT`/`NOLINTNEXTLINE` comments without check list, so I suppose it's fair 
to claim that this comment is redundant (we have already checked that no single 
check reported diagnostics on the line). The `else`-branch reports the 
diagnostics for the definite check in a list in case of `NOLINT(foo, bar)` 
(actually, if neither `foo` nor `bar` checks reported diagnostics for the line, 
there will be a few diagnostics from `nolint-usage` - not sure if it's good, 
but it seems acceptable). That is why, I suppose, it is necessary to distinct 
these cases.

> One concern I have with this functionality is: how should users silence a 
> lint diagnostic that's target sensitive? e.g., a diagnostic that triggers 
> based on 

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 128091.
xgsa marked an inline comment as done.
xgsa added a comment.

Review comments applied.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41326

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  test/clang-tidy/nolint-usage.cpp

Index: test/clang-tidy/nolint-usage.cpp
===
--- test/clang-tidy/nolint-usage.cpp
+++ test/clang-tidy/nolint-usage.cpp
@@ -22,19 +22,19 @@
 // Case: NO_LINT for unknown check on line with an error on some check.
 class A6 { A6(int i); }; // NOLINT(unknown-check)
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
-// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: unknown check name 'unknown-check' is specified for NOLINT comment
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: unknown check name 'unknown-check' specified in NOLINT comment
 
 // Case: NO_LINT for unknown check on line without errors.
 class A7 { explicit A7(int i); }; // NOLINT(unknown-check)
-// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: unknown check name 'unknown-check' is specified for NOLINT comment
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: unknown check name 'unknown-check' specified in NOLINT comment
 
 // Case: NO_LINT with not closed parenthesis without check names.
 class A8 { A8(int i); }; // NOLINT(
-// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: NOLINT comment has an opening parenthesis and no closing one, so all the diagnostics will be silenced for the line
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: unbalanced parentheses in NOLINT comment; all diagnostics silenced for this line
 
 // Case: NO_LINT with not closed parenthesis with check names.
 class A9 { A9(int i); }; // NOLINT(unknown-check
-// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: NOLINT comment has an opening parenthesis and no closing one, so all the diagnostics will be silenced for the line
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: unbalanced parentheses in NOLINT comment; all diagnostics silenced for this line
 
 // Case: NO_LINT with clang diagnostics
 int f() {
@@ -102,23 +102,23 @@
 // Case: NO_LINT_NEXT_LINE for unknown check before line with an error on some check.
 // NOLINTNEXTLINE(unknown-check)
 class C6 { C6(int i); };
-// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: unknown check name 'unknown-check' is specified for NOLINTNEXTLINE comment
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: unknown check name 'unknown-check' specified in NOLINTNEXTLINE comment
 // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: single-argument constructors must be marked explicit
 
 // Case: NO_LINT_NEXT_LINE for unknown check before line without errors.
 // NOLINTNEXTLINE(unknown-check)
 class C7 { explicit C7(int i); };
-// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: unknown check name 'unknown-check' is specified for NOLINTNEXTLINE comment
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: unknown check name 'unknown-check' specified in NOLINTNEXTLINE comment
 
 // Case: NO_LINT_NEXT_LINE with not closed parenthesis without check names.
 // NOLINTNEXTLINE(
 class C8 { C8(int i); };
-// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: NOLINTNEXTLINE comment has an opening parenthesis and no closing one, so all the diagnostics will be silenced for the line
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: unbalanced parentheses in NOLINTNEXTLINE comment; all diagnostics silenced for this line
 
 // Case: NO_LINT_NEXT_LINE with not closed parenthesis with check names.
 // NOLINTNEXTLINE(unknown-check
 class C9 { C9(int i); };
-// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: NOLINTNEXTLINE comment has an opening parenthesis and no closing one, so all the diagnostics will be silenced for the line
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: unbalanced parentheses in NOLINTNEXTLINE comment; all diagnostics silenced for this line
 
 // Case: NO_LINT_NEXT_LINE with clang diagnostics
 int g() {
Index: clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -27,6 +27,7 @@
 class ASTContext;
 class CompilerInstance;
 class Preprocessor;
+
 namespace ast_matchers {
 class MatchFinder;
 }
@@ -202,7 +203,7 @@
   }
 
 private:
-  // Calls setDiagnosticsEngine(), storeError() and getNolintCollector().
+  // Calls setDiagnosticsEngine(), storeError(), and getNolintCollector().
   friend class ClangTidyDiagnosticConsumer;
   friend class ClangTidyPluginAction;
 
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -16,15 +16,16 @@
 ///
 //===--===//
 
-#include "ClangTidyDiagnosticConsumer.h"
-#include "ClangTidyOptions.h"
 

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 128092.
xgsa added a comment.

The full diff (but not only the incremental one) was uploaded. Please, skip 
previous revision. Sorry.


https://reviews.llvm.org/D41326

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  test/clang-tidy/nolint-usage.cpp
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -24,6 +24,18 @@
 class C5 { C5(int i); };
 
 
+void f() {
+  int i;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
+  // NOLINTNEXTLINE
+  int j;
+  // NOLINTNEXTLINE(clang-diagnostic-unused-variable)
+  int k;
+  // NOLINTNEXTLINE(clang-diagnostic-literal-conversion)
+  int l;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
+}
+
 // NOLINTNEXTLINE
 
 class D { D(int i); };
@@ -44,6 +56,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 10 warnings (10 NOLINT)
 
-// RUN: %check_clang_tidy %s google-explicit-constructor %t --
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable %t -- -extra-arg=-Wunused-variable --
Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -30,6 +30,9 @@
   int i;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
   int j; // NOLINT
+  int k; // NOLINT(clang-diagnostic-unused-variable)
+  int l; // NOLINT(clang-diagnostic-literal-conversion)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
 }
 
 #define MACRO(X) class X { X(int i); };
@@ -46,4 +49,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
+// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)
Index: test/clang-tidy/nolint-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/nolint-usage.cpp
@@ -0,0 +1,171 @@
+// RUN: %check_clang_tidy %s nolint-usage,google-explicit-constructor,misc-unused-parameters %t --
+
+// Case: NO_LINT on line with an error.
+class A1 { A1(int i); }; // NOLINT
+
+// Case: NO_LINT on line without errors.
+class A2 { explicit A2(int i); }; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for the specific check on line with an error on it.
+class A3 { A3(int i); }; // NOLINT(google-explicit-constructor)
+
+// Case: NO_LINT for the specific check on line with an error on another check.
+class A4 { A4(int i); }; // NOLINT(misc-unused-parameters)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: there is no diagnostics for the 'misc-unused-parameters' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for the specific check on line without errors.
+class A5 { explicit A5(int i); }; // NOLINT(google-explicit-constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there is no diagnostics for the 'google-explicit-constructor' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for unknown check on line with an error on some check.
+class A6 { A6(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: unknown check name 'unknown-check' specified in NOLINT comment
+
+// Case: NO_LINT for unknown check on line without errors.
+class A7 { explicit A7(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: unknown check name 'unknown-check' specified in NOLINT comment
+
+// Case: NO_LINT with not closed parenthesis without check names.
+class A8 { A8(int i); }; // NOLINT(
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: unbalanced parentheses in NOLINT comment; all diagnostics silenced for this line
+
+// Case: NO_LINT with not closed parenthesis with check names.
+class A9 { A9(int i); }; // NOLINT(unknown-check
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: unbalanced parentheses in NOLINT comment; all diagnostics silenced for this line
+
+// Case: NO_LINT with clang diagnostics
+int f() {
+  int i = 0; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+  int j = 0; // NOLINT(clang-diagnostic-unused-variable)
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there is no diagnostics for the 'clang-diagnostic-unus

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa marked 5 inline comments as done.
xgsa added inline comments.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:276
+
+  using NolintMap = std::unordered_map xgsa wrote:
> > aaron.ballman wrote:
> > > Is there a better LLVM ADT to use here?
> > This data structure provides the fast lookup by check name+line number and 
> > it's exactly what is necessary. What are the concerns about this data 
> > structure?
> Same as above.
I have reviewed llvm guide [1] and found that it recommends using ordered 
vector in such cases. I have implemented this approach, however I'd like to 
notice that lookup complexity in `reportRedundantNolintComments()` increased 
from average O(1) for `std::unordered_map` to O(log(N)) for binary search. 
However, memory usage become less. As this collection is gathered for each 
translation unit, I don't expect millions of `NOLINT` comments in it, thus this 
approach looks suitable.

[1] - 
http://llvm.org/docs/ProgrammersManual.html#picking-the-right-data-structure-for-a-task



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:424
+// This method is not const, because it fills cache on first demand.
+// It is possible to fill cache in constructor or make cache volatile
+// and make this method const, but they seem like worse solutions.

aaron.ballman wrote:
> xgsa wrote:
> > aaron.ballman wrote:
> > > Making the cache volatile will have no impact on this.
> > > 
> > > Any reason not to make the cache `mutable`, however? That's quite common 
> > > for implementation details.
> > Sorry, certainly, instead of "volatile" I meant "mutable".
> > 
> > Actually, using of "mutable" violates a constancy contract, as the field is 
> > get modified in a const method. Thus I'd tend to avoid using `mutable`, if 
> > possible, because e.g. in multi-threaded applications these fields require 
> > additional protection/synchronization. Moreover, I see that using of  
> > `mutable` is not very spread in clang-tidy. Thus as, currently, `hasCheck` 
> > is called from the non-constant context, I'd prefer leaving it non-const 
> > instead of making cache `mutable`. Please, let me know if you insist on the 
> > `mutable` option.
> Use of mutable does not violate constancy; the cache is not exposed via any 
> interface; it is purely an implementation detail. Very little of our code 
> base is concerned with multithreaded scenarios (we use bit-fields 
> *everywhere*, for instance).
> 
> I won't insist on using `mutable` if you are set against it, but this is the 
> exact scenario in which it is the correct solution.
I've just tried the `mutalbe`-approach and discovered one more issue: 
`ClangTidyASTConsumerFactory` requires non constant `ClangTidyContext`. Thus, 
if current approach is not suitable, either `ClangTidyASTConsumerFactory` 
should be refactored on const and nonconst parts or caching should be done in 
constructor (which makes  fetching the names not lazy). Because of this, 
currently I am suggesting to leave `hasCheck` nonconstant.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:839-840
+case NolintCommentType::Nolint:
+  Message = "there is no diagnostics on this line, "
+"the NOLINT comment is redundant";
+  break;

aaron.ballman wrote:
> xgsa wrote:
> > aaron.ballman wrote:
> > > I don't think the user is going to care about the distinction between no 
> > > diagnostics being triggered and the expected diagnostic not being 
> > > triggered. Also, it's dangerous to claim the lint comment is redundant 
> > > because it's possible the user has NOLINT(foo, bar) and while foo is not 
> > > triggered, bar still is. The NOLINT comment itself isn't redundant, it's 
> > > that the check specified doesn't occur.
> > > 
> > > I would consolidate those scenarios into a single diagnostic: "expected 
> > > diagnostic '%0' not generated" and "expected diagnostic '%0' not 
> > > generated for the following line".
> > > 
> > > One concern I have with this functionality is: how should users silence a 
> > > lint diagnostic that's target sensitive? e.g., a diagnostic that triggers 
> > > based on the underlying type of size_t or the signedness of plain char. 
> > > In that case, the diagnostic may trigger for some targets but not others, 
> > > but on the targets where the diagnostic is not triggered, they now get a 
> > > diagnostic they cannot silence. There should be a way to silence the "bad 
> > > NOLINT" diagnostics.
> > > I don't think the user is going to care about the distinction between no 
> > > diagnostics being triggered and the expected diagnostic not being 
> > > triggered. Also, it's dangerous to claim the lint comment is redundant 
> > > because it's possible the user has NOLINT(foo, bar) and while foo is not 
> > > triggered, bar still is. The NOLINT comment itself isn't redundant, it's 
> > > that the check specified doesn't occur.
> > > 
> > 

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 128144.
xgsa marked an inline comment as done.
xgsa added a comment.
Herald added a subscriber: mgrang.

Review comments applied.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41326

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  test/clang-tidy/nolint-usage.cpp
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -24,6 +24,18 @@
 class C5 { C5(int i); };
 
 
+void f() {
+  int i;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
+  // NOLINTNEXTLINE
+  int j;
+  // NOLINTNEXTLINE(clang-diagnostic-unused-variable)
+  int k;
+  // NOLINTNEXTLINE(clang-diagnostic-literal-conversion)
+  int l;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
+}
+
 // NOLINTNEXTLINE
 
 class D { D(int i); };
@@ -44,6 +56,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 10 warnings (10 NOLINT)
 
-// RUN: %check_clang_tidy %s google-explicit-constructor %t --
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable %t -- -extra-arg=-Wunused-variable --
Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -30,6 +30,9 @@
   int i;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
   int j; // NOLINT
+  int k; // NOLINT(clang-diagnostic-unused-variable)
+  int l; // NOLINT(clang-diagnostic-literal-conversion)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
 }
 
 #define MACRO(X) class X { X(int i); };
@@ -46,4 +49,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
+// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)
Index: test/clang-tidy/nolint-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/nolint-usage.cpp
@@ -0,0 +1,171 @@
+// RUN: %check_clang_tidy %s nolint-usage,google-explicit-constructor,misc-unused-parameters %t --
+
+// Case: NO_LINT on line with an error.
+class A1 { A1(int i); }; // NOLINT
+
+// Case: NO_LINT on line without errors.
+class A2 { explicit A2(int i); }; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for the specific check on line with an error on it.
+class A3 { A3(int i); }; // NOLINT(google-explicit-constructor)
+
+// Case: NO_LINT for the specific check on line with an error on another check.
+class A4 { A4(int i); }; // NOLINT(misc-unused-parameters)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: there is no diagnostics for the 'misc-unused-parameters' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for the specific check on line without errors.
+class A5 { explicit A5(int i); }; // NOLINT(google-explicit-constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there is no diagnostics for the 'google-explicit-constructor' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for unknown check on line with an error on some check.
+class A6 { A6(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: unknown check name 'unknown-check' specified in NOLINT comment
+
+// Case: NO_LINT for unknown check on line without errors.
+class A7 { explicit A7(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: unknown check name 'unknown-check' specified in NOLINT comment
+
+// Case: NO_LINT with not closed parenthesis without check names.
+class A8 { A8(int i); }; // NOLINT(
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: unbalanced parentheses in NOLINT comment; all diagnostics silenced for this line
+
+// Case: NO_LINT with not closed parenthesis with check names.
+class A9 { A9(int i); }; // NOLINT(unknown-check
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: unbalanced parentheses in NOLINT comment; all diagnostics silenced for this line
+
+// Case: NO_LINT with clang diagnostics
+int f() {
+  int i = 0; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+  int j = 0; // NOLINT(clang-diagnostic-unused-variable)
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there is no diagnost

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa added inline comments.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:839-840
+case NolintCommentType::Nolint:
+  Message = "there is no diagnostics on this line, "
+"the NOLINT comment is redundant";
+  break;

aaron.ballman wrote:
> xgsa wrote:
> > aaron.ballman wrote:
> > > xgsa wrote:
> > > > aaron.ballman wrote:
> > > > > I don't think the user is going to care about the distinction between 
> > > > > no diagnostics being triggered and the expected diagnostic not being 
> > > > > triggered. Also, it's dangerous to claim the lint comment is 
> > > > > redundant because it's possible the user has NOLINT(foo, bar) and 
> > > > > while foo is not triggered, bar still is. The NOLINT comment itself 
> > > > > isn't redundant, it's that the check specified doesn't occur.
> > > > > 
> > > > > I would consolidate those scenarios into a single diagnostic: 
> > > > > "expected diagnostic '%0' not generated" and "expected diagnostic 
> > > > > '%0' not generated for the following line".
> > > > > 
> > > > > One concern I have with this functionality is: how should users 
> > > > > silence a lint diagnostic that's target sensitive? e.g., a diagnostic 
> > > > > that triggers based on the underlying type of size_t or the 
> > > > > signedness of plain char. In that case, the diagnostic may trigger 
> > > > > for some targets but not others, but on the targets where the 
> > > > > diagnostic is not triggered, they now get a diagnostic they cannot 
> > > > > silence. There should be a way to silence the "bad NOLINT" 
> > > > > diagnostics.
> > > > > I don't think the user is going to care about the distinction between 
> > > > > no diagnostics being triggered and the expected diagnostic not being 
> > > > > triggered. Also, it's dangerous to claim the lint comment is 
> > > > > redundant because it's possible the user has NOLINT(foo, bar) and 
> > > > > while foo is not triggered, bar still is. The NOLINT comment itself 
> > > > > isn't redundant, it's that the check specified doesn't occur.
> > > > > 
> > > > > I would consolidate those scenarios into a single diagnostic: 
> > > > > "expected diagnostic '%0' not generated" and "expected diagnostic 
> > > > > '%0' not generated for the following line".
> > > > 
> > > > This branch of `if (NolintEntry.first.CheckName == 
> > > > NolintCommentsCollector::AnyCheck)` reports only about 
> > > > `NOLINT`/`NOLINTNEXTLINE` comments without check list, so I suppose 
> > > > it's fair to claim that this comment is redundant (we have already 
> > > > checked that no single check reported diagnostics on the line). The 
> > > > `else`-branch reports the diagnostics for the definite check in a list 
> > > > in case of `NOLINT(foo, bar)` (actually, if neither `foo` nor `bar` 
> > > > checks reported diagnostics for the line, there will be a few 
> > > > diagnostics from `nolint-usage` - not sure if it's good, but it seems 
> > > > acceptable). That is why, I suppose, it is necessary to distinct these 
> > > > cases.
> > > > 
> > > > > One concern I have with this functionality is: how should users 
> > > > > silence a lint diagnostic that's target sensitive? e.g., a diagnostic 
> > > > > that triggers based on the underlying type of size_t or the 
> > > > > signedness of plain char. In that case, the diagnostic may trigger 
> > > > > for some targets but not others, but on the targets where the 
> > > > > diagnostic is not triggered, they now get a diagnostic they cannot 
> > > > > silence. There should be a way to silence the "bad NOLINT" 
> > > > > diagnostics.
> > > > 
> > > > There is such mechanism: it is possible to specify `// 
> > > > NOLINT(nolint-usage)` or `//NOLINT(check1, check2, nolint-usage) to 
> > > > silence the `nolint-usage`-mechanism. Please, see tests for details and 
> > > > more examples. 
> > > Can you provide an example where this distinction will make a difference 
> > > to the user and help clarify a confusing situation? I cannot think of 
> > > one, and it would be nice to simplify this code.
> > > 
> > > Thank you for the explanation about nolint-usage. This is not terminology 
> > > I've seen before -- is this your invention, or is it a documented feature 
> > > for NOLINT comments?
> > >> Can you provide an example where this distinction will make a difference 
> > >> to the user and help clarify a confusing situation? I cannot think of 
> > >> one, and it would be nice to simplify this code.
> > 
> > Example for the diagnostics emitted in the `if`-branch:
> > ```
> > class A2 { explicit A2(int i); }; // NOLINT
> > => warning: there is no diagnostics on this line, the NOLINT comment is 
> > redundant
> > ```
> > Thus, the whole NOLINT comment should be removed.
> > 
> > Example for the diagnostics emitted in the `else`-branch:
> > ```
> > // Case: NO_LINT for the specific check on line with an error on another 
> > check.
> > class A4 { A4(int i); }; // NOLIN

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 128431.
xgsa added a comment.

Rename the check `nolint-usage` => `readability-nolint-usage` for consistency.
Update diagnostics message according to the review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41326

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp
  test/clang-tidy/readability-nolint-usage.cpp

Index: test/clang-tidy/readability-nolint-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-nolint-usage.cpp
@@ -0,0 +1,171 @@
+// RUN: %check_clang_tidy %s readability-nolint-usage,google-explicit-constructor,misc-unused-parameters %t --
+
+// Case: NO_LINT on line with an error.
+class A1 { A1(int i); }; // NOLINT
+
+// Case: NO_LINT on line without errors.
+class A2 { explicit A2(int i); }; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there are no diagnostics on this line
+
+// Case: NO_LINT for the specific check on line with an error on it.
+class A3 { A3(int i); }; // NOLINT(google-explicit-constructor)
+
+// Case: NO_LINT for the specific check on line with an error on another check.
+class A4 { A4(int i); }; // NOLINT(misc-unused-parameters)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: there are no diagnostics for 'misc-unused-parameters' on this line
+
+// Case: NO_LINT for the specific check on line without errors.
+class A5 { explicit A5(int i); }; // NOLINT(google-explicit-constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there are no diagnostics for 'google-explicit-constructor' on this line
+
+// Case: NO_LINT for unknown check on line with an error on some check.
+class A6 { A6(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: unknown check name 'unknown-check' specified in NOLINT comment
+
+// Case: NO_LINT for unknown check on line without errors.
+class A7 { explicit A7(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: unknown check name 'unknown-check' specified in NOLINT comment
+
+// Case: NO_LINT with not closed parenthesis without check names.
+class A8 { A8(int i); }; // NOLINT(
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: unbalanced parentheses in NOLINT comment; all diagnostics silenced for this line
+
+// Case: NO_LINT with not closed parenthesis with check names.
+class A9 { A9(int i); }; // NOLINT(unknown-check
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: unbalanced parentheses in NOLINT comment; all diagnostics silenced for this line
+
+// Case: NO_LINT with clang diagnostics
+int f() {
+  int i = 0; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there are no diagnostics on this line
+  int j = 0; // NOLINT(clang-diagnostic-unused-variable)
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there are no diagnostics for 'clang-diagnostic-unused-variable' on this line
+  return i + j;
+}
+
+#define MACRO(X) class X { explicit X(int i); };
+
+// Case: NO_LINT on macro instantiation line
+MACRO(M1) // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: there are no diagnostics on this line
+
+// Case: NO_LINT with the specific check on macro instantiation line
+MACRO(M2) // NOLINT(google-explicit-constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: there are no diagnostics for 'google-explicit-constructor' on this line
+
+// Case: NO_LINT on macro line
+#define MACRO2(X) class X { explicit X(int i); }; // NOLINT
+MACRO2(M3)
+// CHECK-MESSAGES: :[[@LINE-2]]:54: warning: there are no diagnostics on this line
+
+// Case: NO_LINT with the specific check on macro line
+#define MACRO3(X) class X { explicit X(int i); }; // NOLINT(google-explicit-constructor)
+MACRO(M4)
+// CHECK-MESSAGES: :[[@LINE-2]]:54: warning: there are no diagnostics for 'google-explicit-constructor' on this line
+
+// Case: NO_LINT with readability-nolint-usage on line without errors.
+class B1 { explicit B1(int i); }; // NOLINT(readability-nolint-usage)
+
+// Case: NO_LINT with the specific check and readability-nolint-usage on line without errors.
+class B2 { explicit B2(int i); }; // NOLINT(google-explicit-constructor, readability-nolint-usage)
+
+// Case: NO_LINT with the unknown check and readability-nolint-usage on line without errors.
+class B3 { explicit B3(int i); }; // NOLINT(unknown-check, readability-nolint-usage)
+
+
+// Case: NO_LINT_NEXT_LINE before line with an error.
+// NOLINTNEXTLINE
+class C1 { C1(int i); };
+
+// Case: NO_LINT_NEXT_LINE before line without errors.
+// NOLINTNEXTLINE
+class C2 { explicit C2(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: there are no diagnostics on the next line
+
+// Case: NO_LINT_NEXT

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa added inline comments.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:839-840
+case NolintCommentType::Nolint:
+  Message = "there is no diagnostics on this line, "
+"the NOLINT comment is redundant";
+  break;

aaron.ballman wrote:
> xgsa wrote:
> > aaron.ballman wrote:
> > > xgsa wrote:
> > > > aaron.ballman wrote:
> > > > > xgsa wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > I don't think the user is going to care about the distinction 
> > > > > > > between no diagnostics being triggered and the expected 
> > > > > > > diagnostic not being triggered. Also, it's dangerous to claim the 
> > > > > > > lint comment is redundant because it's possible the user has 
> > > > > > > NOLINT(foo, bar) and while foo is not triggered, bar still is. 
> > > > > > > The NOLINT comment itself isn't redundant, it's that the check 
> > > > > > > specified doesn't occur.
> > > > > > > 
> > > > > > > I would consolidate those scenarios into a single diagnostic: 
> > > > > > > "expected diagnostic '%0' not generated" and "expected diagnostic 
> > > > > > > '%0' not generated for the following line".
> > > > > > > 
> > > > > > > One concern I have with this functionality is: how should users 
> > > > > > > silence a lint diagnostic that's target sensitive? e.g., a 
> > > > > > > diagnostic that triggers based on the underlying type of size_t 
> > > > > > > or the signedness of plain char. In that case, the diagnostic may 
> > > > > > > trigger for some targets but not others, but on the targets where 
> > > > > > > the diagnostic is not triggered, they now get a diagnostic they 
> > > > > > > cannot silence. There should be a way to silence the "bad NOLINT" 
> > > > > > > diagnostics.
> > > > > > > I don't think the user is going to care about the distinction 
> > > > > > > between no diagnostics being triggered and the expected 
> > > > > > > diagnostic not being triggered. Also, it's dangerous to claim the 
> > > > > > > lint comment is redundant because it's possible the user has 
> > > > > > > NOLINT(foo, bar) and while foo is not triggered, bar still is. 
> > > > > > > The NOLINT comment itself isn't redundant, it's that the check 
> > > > > > > specified doesn't occur.
> > > > > > > 
> > > > > > > I would consolidate those scenarios into a single diagnostic: 
> > > > > > > "expected diagnostic '%0' not generated" and "expected diagnostic 
> > > > > > > '%0' not generated for the following line".
> > > > > > 
> > > > > > This branch of `if (NolintEntry.first.CheckName == 
> > > > > > NolintCommentsCollector::AnyCheck)` reports only about 
> > > > > > `NOLINT`/`NOLINTNEXTLINE` comments without check list, so I suppose 
> > > > > > it's fair to claim that this comment is redundant (we have already 
> > > > > > checked that no single check reported diagnostics on the line). The 
> > > > > > `else`-branch reports the diagnostics for the definite check in a 
> > > > > > list in case of `NOLINT(foo, bar)` (actually, if neither `foo` nor 
> > > > > > `bar` checks reported diagnostics for the line, there will be a few 
> > > > > > diagnostics from `nolint-usage` - not sure if it's good, but it 
> > > > > > seems acceptable). That is why, I suppose, it is necessary to 
> > > > > > distinct these cases.
> > > > > > 
> > > > > > > One concern I have with this functionality is: how should users 
> > > > > > > silence a lint diagnostic that's target sensitive? e.g., a 
> > > > > > > diagnostic that triggers based on the underlying type of size_t 
> > > > > > > or the signedness of plain char. In that case, the diagnostic may 
> > > > > > > trigger for some targets but not others, but on the targets where 
> > > > > > > the diagnostic is not triggered, they now get a diagnostic they 
> > > > > > > cannot silence. There should be a way to silence the "bad NOLINT" 
> > > > > > > diagnostics.
> > > > > > 
> > > > > > There is such mechanism: it is possible to specify `// 
> > > > > > NOLINT(nolint-usage)` or `//NOLINT(check1, check2, nolint-usage) to 
> > > > > > silence the `nolint-usage`-mechanism. Please, see tests for details 
> > > > > > and more examples. 
> > > > > Can you provide an example where this distinction will make a 
> > > > > difference to the user and help clarify a confusing situation? I 
> > > > > cannot think of one, and it would be nice to simplify this code.
> > > > > 
> > > > > Thank you for the explanation about nolint-usage. This is not 
> > > > > terminology I've seen before -- is this your invention, or is it a 
> > > > > documented feature for NOLINT comments?
> > > > >> Can you provide an example where this distinction will make a 
> > > > >> difference to the user and help clarify a confusing situation? I 
> > > > >> cannot think of one, and it would be nice to simplify this code.
> > > > 
> > > > Example for the diagnostics emitted in the `if`-branch:
> > > > ```
> > > > class A2 { explicit A2(int i); }; // 

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 128893.
xgsa added a comment.

Fixed showing the check in -list-checks.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41326

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp
  test/clang-tidy/readability-nolint-usage.cpp

Index: test/clang-tidy/readability-nolint-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-nolint-usage.cpp
@@ -0,0 +1,171 @@
+// RUN: %check_clang_tidy %s readability-nolint-usage,google-explicit-constructor,misc-unused-parameters %t --
+
+// Case: NO_LINT on line with an error.
+class A1 { A1(int i); }; // NOLINT
+
+// Case: NO_LINT on line without errors.
+class A2 { explicit A2(int i); }; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there are no diagnostics on this line
+
+// Case: NO_LINT for the specific check on line with an error on it.
+class A3 { A3(int i); }; // NOLINT(google-explicit-constructor)
+
+// Case: NO_LINT for the specific check on line with an error on another check.
+class A4 { A4(int i); }; // NOLINT(misc-unused-parameters)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: there are no diagnostics for 'misc-unused-parameters' on this line
+
+// Case: NO_LINT for the specific check on line without errors.
+class A5 { explicit A5(int i); }; // NOLINT(google-explicit-constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there are no diagnostics for 'google-explicit-constructor' on this line
+
+// Case: NO_LINT for unknown check on line with an error on some check.
+class A6 { A6(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: unknown check name 'unknown-check' specified in NOLINT comment
+
+// Case: NO_LINT for unknown check on line without errors.
+class A7 { explicit A7(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: unknown check name 'unknown-check' specified in NOLINT comment
+
+// Case: NO_LINT with not closed parenthesis without check names.
+class A8 { A8(int i); }; // NOLINT(
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: unbalanced parentheses in NOLINT comment; all diagnostics silenced for this line
+
+// Case: NO_LINT with not closed parenthesis with check names.
+class A9 { A9(int i); }; // NOLINT(unknown-check
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: unbalanced parentheses in NOLINT comment; all diagnostics silenced for this line
+
+// Case: NO_LINT with clang diagnostics
+int f() {
+  int i = 0; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there are no diagnostics on this line
+  int j = 0; // NOLINT(clang-diagnostic-unused-variable)
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there are no diagnostics for 'clang-diagnostic-unused-variable' on this line
+  return i + j;
+}
+
+#define MACRO(X) class X { explicit X(int i); };
+
+// Case: NO_LINT on macro instantiation line
+MACRO(M1) // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: there are no diagnostics on this line
+
+// Case: NO_LINT with the specific check on macro instantiation line
+MACRO(M2) // NOLINT(google-explicit-constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: there are no diagnostics for 'google-explicit-constructor' on this line
+
+// Case: NO_LINT on macro line
+#define MACRO2(X) class X { explicit X(int i); }; // NOLINT
+MACRO2(M3)
+// CHECK-MESSAGES: :[[@LINE-2]]:54: warning: there are no diagnostics on this line
+
+// Case: NO_LINT with the specific check on macro line
+#define MACRO3(X) class X { explicit X(int i); }; // NOLINT(google-explicit-constructor)
+MACRO(M4)
+// CHECK-MESSAGES: :[[@LINE-2]]:54: warning: there are no diagnostics for 'google-explicit-constructor' on this line
+
+// Case: NO_LINT with readability-nolint-usage on line without errors.
+class B1 { explicit B1(int i); }; // NOLINT(readability-nolint-usage)
+
+// Case: NO_LINT with the specific check and readability-nolint-usage on line without errors.
+class B2 { explicit B2(int i); }; // NOLINT(google-explicit-constructor, readability-nolint-usage)
+
+// Case: NO_LINT with the unknown check and readability-nolint-usage on line without errors.
+class B3 { explicit B3(int i); }; // NOLINT(unknown-check, readability-nolint-usage)
+
+
+// Case: NO_LINT_NEXT_LINE before line with an error.
+// NOLINTNEXTLINE
+class C1 { C1(int i); };
+
+// Case: NO_LINT_NEXT_LINE before line without errors.
+// NOLINTNEXTLINE
+class C2 { explicit C2(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: there are no diagnostics on the next line
+
+// Case: NO_LINT_NEXT_LINE for the specific check before line with an error on it.
+// NOLINTNEXTLINE(google-explicit-co

[PATCH] D40671: Support specific categories for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa created this revision.
xgsa added a project: clang-tools-extra.

The NOLINT directive was extended to support the "NOLINT(category)" and 
"NOLINT(*)" syntax. Also it is possible to specify a few categories separated 
with comma "NOLINT(cat1, cat2)"


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40671

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -13,8 +13,17 @@
 
 class B { B(int i); }; // NOLINT
 
-class C { C(int i); }; // NOLINT(we-dont-care-about-categories-yet)
+class C { C(int i); }; // NOLINT(for-some-other-category)
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-category, google-explicit-constructor)
+
 void f() {
   int i;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
@@ -35,4 +44,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 11 warnings (11 NOLINT)
Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -4,10 +4,23 @@
 // NOLINTNEXTLINE
 class B { B(int i); };
 
-// NOLINTNEXTLINE(we-dont-care-about-categories-yet)
+// NOLINTNEXTLINE(for-some-other-category)
 class C { C(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
 
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-category, google-explicit-constructor)
+class C4 { C4(int i); };
+
+
 // NOLINTNEXTLINE
 
 class D { D(int i); };
@@ -28,6 +41,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 4 warnings (4 NOLINT)
+// CHECK-MESSAGES: Suppressed 7 warnings (7 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -22,6 +22,7 @@
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
 #include "llvm/ADT/SmallString.h"
+#include 
 #include 
 #include 
 using namespace clang;
@@ -289,8 +290,37 @@
   LastErrorRelatesToUserCode = false;
   LastErrorPassesLineFilter = false;
 }
-
-static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc) {
+static bool IsNOLINTFound(StringRef NolintMacro, StringRef Line,
+  unsigned DiagID, const ClangTidyContext &Context) {
+  const auto NolintIndex = Line.find(NolintMacro);
+  if (NolintIndex != StringRef::npos) {
+auto BracketIndex = NolintIndex + NolintMacro.size();
+if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
+  ++BracketIndex;
+  const auto BracketEndIndex = Line.find(')', BracketIndex);
+  if (BracketEndIndex != StringRef::npos) {
+auto ChecksStr =
+Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
+if (ChecksStr != "*") {
+  auto CheckName = Context.getCheckName(DiagID);
+  // Allow specifying a few check names, delimited with comma
+  SmallVector Checks;
+  ChecksStr.split(Checks, ',', -1, false);
+  for (auto &Check : Checks) {
+Check = Check.trim();
+  }
+  return std::find(Checks.begin(), Checks.end(), CheckName) !=
+ Checks.end();
+}
+  }
+}
+return true;
+  }
+  return false;
+}
+static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc,
+   unsigned DiagID,
+   const ClangTidyContext &Context) {
   bool Invalid;
   const char *CharacterData = SM.getCharacterData(Loc, &Invalid);
   if (Invalid)
@@ -301,8 +331,7 @@
   while (*P != '\0' && *P != '\r' && *P != '\n')
 ++P;
   StringRef RestOfLine(CharacterData, P - CharacterData + 1);
-  // FIXME: Handle /\bNOLINT\b(\([^)]*\))?/ as cpplint.py does.
-  if (RestOfLine.find("NOLINT") != StringRef::npos)
+  if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context))
 return true;
 
   // Check if there's a NOLINTNEXTLINE on the previous line.
@@ -329,16 +358,17 @@
 --P;
 
   RestOfLine = StringRef(P, LineEnd -

[PATCH] D40671: Support specific categories for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 125008.
xgsa added a reviewer: dcoughlin.
xgsa added a comment.

Update the diff to contain the full context


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40671

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -13,7 +13,16 @@
 
 class B { B(int i); }; // NOLINT
 
-class C { C(int i); }; // NOLINT(we-dont-care-about-categories-yet)
+class C { C(int i); }; // NOLINT(for-some-other-category)
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-category, google-explicit-constructor)
 
 void f() {
   int i;
@@ -35,4 +44,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 11 warnings (11 NOLINT)
Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -4,8 +4,21 @@
 // NOLINTNEXTLINE
 class B { B(int i); };
 
-// NOLINTNEXTLINE(we-dont-care-about-categories-yet)
+// NOLINTNEXTLINE(for-some-other-category)
 class C { C(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-category, google-explicit-constructor)
+class C4 { C4(int i); };
 
 
 // NOLINTNEXTLINE
@@ -28,6 +41,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 4 warnings (4 NOLINT)
+// CHECK-MESSAGES: Suppressed 7 warnings (7 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -22,6 +22,7 @@
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
 #include "llvm/ADT/SmallString.h"
+#include 
 #include 
 #include 
 using namespace clang;
@@ -289,8 +290,37 @@
   LastErrorRelatesToUserCode = false;
   LastErrorPassesLineFilter = false;
 }
-
-static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc) {
+static bool IsNOLINTFound(StringRef NolintMacro, StringRef Line,
+  unsigned DiagID, const ClangTidyContext &Context) {
+  const auto NolintIndex = Line.find(NolintMacro);
+  if (NolintIndex != StringRef::npos) {
+auto BracketIndex = NolintIndex + NolintMacro.size();
+if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
+  ++BracketIndex;
+  const auto BracketEndIndex = Line.find(')', BracketIndex);
+  if (BracketEndIndex != StringRef::npos) {
+auto ChecksStr =
+Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
+if (ChecksStr != "*") {
+  auto CheckName = Context.getCheckName(DiagID);
+  // Allow specifying a few check names, delimited with comma
+  SmallVector Checks;
+  ChecksStr.split(Checks, ',', -1, false);
+  for (auto &Check : Checks) {
+Check = Check.trim();
+  }
+  return std::find(Checks.begin(), Checks.end(), CheckName) !=
+ Checks.end();
+}
+  }
+}
+return true;
+  }
+  return false;
+}
+static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc,
+   unsigned DiagID,
+   const ClangTidyContext &Context) {
   bool Invalid;
   const char *CharacterData = SM.getCharacterData(Loc, &Invalid);
   if (Invalid)
@@ -301,8 +331,7 @@
   while (*P != '\0' && *P != '\r' && *P != '\n')
 ++P;
   StringRef RestOfLine(CharacterData, P - CharacterData + 1);
-  // FIXME: Handle /\bNOLINT\b(\([^)]*\))?/ as cpplint.py does.
-  if (RestOfLine.find("NOLINT") != StringRef::npos)
+  if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context))
 return true;
 
   // Check if there's a NOLINTNEXTLINE on the previous line.
@@ -329,16 +358,17 @@
 --P;
 
   RestOfLine = StringRef(P, LineEnd - P + 1);
-  if (RestOfLine.find("NOLINTNEXTLINE") != StringRef::npos)
+  if (IsNOLINTFound("NOLINTNEXTLINE", RestOfLine, DiagID, Context))
 return true;
 
   return false;
 }
 
-static bool LineIsMarkedWithNOLINTinMacro(SourceManager

[PATCH] D40671: [clang-tidy] Support specific categories for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa added a comment.

In https://reviews.llvm.org/D40671#941676, @JonasToth wrote:

> Could you please explain what category means? Could i disable all of 
> `cppcoreguidelines` with something like `// NOLINT (cppcoreguidelines-*)`?


No, only individual checks can be disabled. You are right, by "categories" I 
meant just "checks". Sorry for confusion. I'll update the description.




Comment at: test/clang-tidy/nolintnextline.cpp:14
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };

JonasToth wrote:
> missing `)`
No, it's intentionally: it's a test for case when `)` is missing


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40671



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

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa added inline comments.



Comment at: test/clang-tidy/nolintnextline.cpp:14
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };

JonasToth wrote:
> xgsa wrote:
> > JonasToth wrote:
> > > missing `)`
> > No, it's intentionally: it's a test for case when `)` is missing
> It was early, i didn't read properly. Sorry for that.
> 
> A testcase for where all parens are missing would be nice, too. I assume that 
> everything will be ignored, is that correct?
There is such test case (for class "B") or did you mean something else?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40671



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

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 125084.
xgsa added a comment.
Herald added a subscriber: xazax.hun.

Add comments to code as it was recommended.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40671

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -13,7 +13,16 @@
 
 class B { B(int i); }; // NOLINT
 
-class C { C(int i); }; // NOLINT(we-dont-care-about-categories-yet)
+class C { C(int i); }; // NOLINT(for-some-other-category)
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-category, google-explicit-constructor)
 
 void f() {
   int i;
@@ -35,4 +44,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 11 warnings (11 NOLINT)
Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -4,8 +4,21 @@
 // NOLINTNEXTLINE
 class B { B(int i); };
 
-// NOLINTNEXTLINE(we-dont-care-about-categories-yet)
+// NOLINTNEXTLINE(for-some-other-category)
 class C { C(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-category, google-explicit-constructor)
+class C4 { C4(int i); };
 
 
 // NOLINTNEXTLINE
@@ -28,6 +41,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 4 warnings (4 NOLINT)
+// CHECK-MESSAGES: Suppressed 7 warnings (7 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -22,6 +22,7 @@
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
 #include "llvm/ADT/SmallString.h"
+#include 
 #include 
 #include 
 using namespace clang;
@@ -289,8 +290,39 @@
   LastErrorRelatesToUserCode = false;
   LastErrorPassesLineFilter = false;
 }
-
-static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc) {
+static bool IsNOLINTFound(StringRef NolintMacro, StringRef Line,
+  unsigned DiagID, const ClangTidyContext &Context) {
+  const auto NolintIndex = Line.find(NolintMacro);
+  if (NolintIndex != StringRef::npos) {
+auto BracketIndex = NolintIndex + NolintMacro.size();
+// Check if the specific checks are specified in brackets
+if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
+  ++BracketIndex;
+  const auto BracketEndIndex = Line.find(')', BracketIndex);
+  if (BracketEndIndex != StringRef::npos) {
+auto ChecksStr =
+Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
+// Allow disabling all the checks with "*"
+if (ChecksStr != "*") {
+  auto CheckName = Context.getCheckName(DiagID);
+  // Allow specifying a few check names, delimited with comma
+  SmallVector Checks;
+  ChecksStr.split(Checks, ',', -1, false);
+  for (auto &Check : Checks) {
+Check = Check.trim();
+  }
+  return std::find(Checks.begin(), Checks.end(), CheckName) !=
+ Checks.end();
+}
+  }
+}
+return true;
+  }
+  return false;
+}
+static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc,
+   unsigned DiagID,
+   const ClangTidyContext &Context) {
   bool Invalid;
   const char *CharacterData = SM.getCharacterData(Loc, &Invalid);
   if (Invalid)
@@ -301,8 +333,7 @@
   while (*P != '\0' && *P != '\r' && *P != '\n')
 ++P;
   StringRef RestOfLine(CharacterData, P - CharacterData + 1);
-  // FIXME: Handle /\bNOLINT\b(\([^)]*\))?/ as cpplint.py does.
-  if (RestOfLine.find("NOLINT") != StringRef::npos)
+  if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context))
 return true;
 
   // Check if there's a NOLINTNEXTLINE on the previous line.
@@ -329,16 +360,17 @@
 --P;
 
   RestOfLine = StringRef(P, LineEnd - P + 1);
-  if (RestOfLine.find("NOLINTNEXTLINE") != StringRef::npos)
+  if (IsNOLINTFound("NOLINTNEXTLINE", RestOfLin

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 125096.
xgsa added a comment.

A few additional test cases were added.


https://reviews.llvm.org/D40671

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -13,7 +13,18 @@
 
 class B { B(int i); }; // NOLINT
 
-class C { C(int i); }; // NOLINT(we-dont-care-about-categories-yet)
+class C { C(int i); }; // NOLINT(for-some-other-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
+
+class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
 
 void f() {
   int i;
@@ -35,4 +46,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -4,8 +4,24 @@
 // NOLINTNEXTLINE
 class B { B(int i); };
 
-// NOLINTNEXTLINE(we-dont-care-about-categories-yet)
+// NOLINTNEXTLINE(for-some-other-check)
 class C { C(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-check, google-explicit-constructor)
+class C4 { C4(int i); };
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };
 
 
 // NOLINTNEXTLINE
@@ -28,6 +44,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 4 warnings (4 NOLINT)
+// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -22,6 +22,7 @@
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
 #include "llvm/ADT/SmallString.h"
+#include 
 #include 
 #include 
 using namespace clang;
@@ -289,8 +290,39 @@
   LastErrorRelatesToUserCode = false;
   LastErrorPassesLineFilter = false;
 }
-
-static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc) {
+static bool IsNOLINTFound(StringRef NolintMacro, StringRef Line,
+  unsigned DiagID, const ClangTidyContext &Context) {
+  const auto NolintIndex = Line.find(NolintMacro);
+  if (NolintIndex != StringRef::npos) {
+auto BracketIndex = NolintIndex + NolintMacro.size();
+// Check if the specific checks are specified in brackets
+if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
+  ++BracketIndex;
+  const auto BracketEndIndex = Line.find(')', BracketIndex);
+  if (BracketEndIndex != StringRef::npos) {
+auto ChecksStr =
+Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
+// Allow disabling all the checks with "*"
+if (ChecksStr != "*") {
+  auto CheckName = Context.getCheckName(DiagID);
+  // Allow specifying a few check names, delimited with comma
+  SmallVector Checks;
+  ChecksStr.split(Checks, ',', -1, false);
+  for (auto &Check : Checks) {
+Check = Check.trim();
+  }
+  return std::find(Checks.begin(), Checks.end(), CheckName) !=
+ Checks.end();
+}
+  }
+}
+return true;
+  }
+  return false;
+}
+static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc,
+   unsigned DiagID,
+   const ClangTidyContext &Context) {
   bool Invalid;
   const char *CharacterData = SM.getCharacterData(Loc, &Invalid);
   if (Invalid)
@@ -301,8 +333,7 @@
   while (*P != '\0' && *P != '\r' && *P != '\n')
 ++P;
   StringRef RestOfLine(CharacterData, P - CharacterData + 1);
-  // FIXME: Handle /\bNOLINT\b(\([^)]*\))?/ as cpplint.py does.
-  if (RestOfLine.find("NOLINT") != StringRef::npos)
+  if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context))
 return true;
 
   // Check if there's a NOLINTNEXTLINE on the previous line.
@@ -329,16 +360,17 @@
 --P;
 
   RestOfLine = StringRef(P, LineEnd - P + 1);
-  if (RestOfLine.find("NOLINTNEXT

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 125140.

https://reviews.llvm.org/D40671

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -13,7 +13,18 @@
 
 class B { B(int i); }; // NOLINT
 
-class C { C(int i); }; // NOLINT(we-dont-care-about-categories-yet)
+class C { C(int i); }; // NOLINT(for-some-other-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
+
+class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
 
 void f() {
   int i;
@@ -35,4 +46,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -4,8 +4,24 @@
 // NOLINTNEXTLINE
 class B { B(int i); };
 
-// NOLINTNEXTLINE(we-dont-care-about-categories-yet)
+// NOLINTNEXTLINE(for-some-other-check)
 class C { C(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-check, google-explicit-constructor)
+class C4 { C4(int i); };
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };
 
 
 // NOLINTNEXTLINE
@@ -28,6 +44,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 4 warnings (4 NOLINT)
+// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include 
 #include 
@@ -290,7 +291,38 @@
   LastErrorPassesLineFilter = false;
 }
 
-static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc) {
+static bool IsNOLINTFound(StringRef NolintMacro, StringRef Line,
+  unsigned DiagID, const ClangTidyContext &Context) {
+  const size_t NolintIndex = Line.find(NolintMacro);
+  if (NolintIndex != StringRef::npos) {
+size_t BracketIndex = NolintIndex + NolintMacro.size();
+// Check if the specific checks are specified in brackets
+if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
+  ++BracketIndex;
+  const size_t BracketEndIndex = Line.find(')', BracketIndex);
+  if (BracketEndIndex != StringRef::npos) {
+StringRef ChecksStr =
+Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
+// Allow disabling all the checks with "*"
+if (ChecksStr != "*") {
+  StringRef CheckName = Context.getCheckName(DiagID);
+  // Allow specifying a few check names, delimited with comma
+  SmallVector Checks;
+  ChecksStr.split(Checks, ',', -1, false);
+  llvm::transform(Checks, Checks.begin(),
+  [](StringRef S) { return S.trim(); });
+  return llvm::find(Checks, CheckName) != Checks.end();
+}
+  }
+}
+return true;
+  }
+  return false;
+}
+
+static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc,
+   unsigned DiagID,
+   const ClangTidyContext &Context) {
   bool Invalid;
   const char *CharacterData = SM.getCharacterData(Loc, &Invalid);
   if (Invalid)
@@ -301,8 +333,7 @@
   while (*P != '\0' && *P != '\r' && *P != '\n')
 ++P;
   StringRef RestOfLine(CharacterData, P - CharacterData + 1);
-  // FIXME: Handle /\bNOLINT\b(\([^)]*\))?/ as cpplint.py does.
-  if (RestOfLine.find("NOLINT") != StringRef::npos)
+  if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context))
 return true;
 
   // Check if there's a NOLINTNEXTLINE on the previous line.
@@ -329,16 +360,17 @@
 --P;
 
   RestOfLine = StringRef(P, LineEnd - P + 1);
-  if (RestOfLine.find("NOLINTNEXTLINE") != StringRef::npos)
+  if (IsNOLINTFound("NOLINTNEXT

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 125190.
xgsa added a comment.

An item to release notes was added.

Also I have added a paragraph about NOLINT to the main documentation page, 
because I suppose it's useful information and it's related to the feature. 
Please, let me know if it should be added with a separate commit or shouldn't 
be added at all.


https://reviews.llvm.org/D40671

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -250,6 +250,27 @@
   value:   'some value'
   ...
 
+Generally, there is no need to suppress :program:`clang-tidy` diagnostics. If
+there are false positives, either a bug should be reported or the code should be
+updated to be clear for both tool and developer. However, if there is a need to
+silent some diagnostics for a line of code, the ``NOLINT`` or ``NOLINTNEXTLINE``
+comments can be used. For example:
+
+.. code-block:: c++
+
+  class Foo
+  {
+// Skip all the diagnostics for the line
+Foo(int param); // NOLINT
+
+// Skip only the specified checks for the line
+Foo(double param); // NOLINT(google-explicit-constructor, google-runtime-int)
+
+// Skip only the specified diagnostics for the next line
+// NOLINTNEXTLINE (google-explicit-constructor, google-runtime-int)
+Foo(bool param); 
+  };
+
 .. _LibTooling: http://clang.llvm.org/docs/LibTooling.html
 .. _How To Setup Tooling For LLVM: http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -256,6 +256,9 @@
   - `hicpp-use-nullptr `_
   - `hicpp-vararg `_
 
+- The ``NOLINT`` and ``NOLINTNEXTLINE`` suppression comments were extended to support the list of checks
+  to disable in parentheses.
+
 Improvements to include-fixer
 -
 
Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -13,7 +13,18 @@
 
 class B { B(int i); }; // NOLINT
 
-class C { C(int i); }; // NOLINT(we-dont-care-about-categories-yet)
+class C { C(int i); }; // NOLINT(for-some-other-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
+
+class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
 
 void f() {
   int i;
@@ -35,4 +46,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -4,8 +4,24 @@
 // NOLINTNEXTLINE
 class B { B(int i); };
 
-// NOLINTNEXTLINE(we-dont-care-about-categories-yet)
+// NOLINTNEXTLINE(for-some-other-check)
 class C { C(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-check, google-explicit-constructor)
+class C4 { C4(int i); };
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };
 
 
 // NOLINTNEXTLINE
@@ -28,6 +44,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 4 warnings (4 NOLINT)
+// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include 
 #include 
@@ -290,7 +291,38 @@
   LastErrorPassesLineFilter = false;
 }
 
-static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc) {
+static bool I

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 125216.
xgsa added a comment.

Minor change: update default value of SmallVector of check names.


https://reviews.llvm.org/D40671

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -250,6 +250,27 @@
   value:   'some value'
   ...
 
+Generally, there is no need to suppress :program:`clang-tidy` diagnostics. If
+there are false positives, either a bug should be reported or the code should be
+updated to be clear for both tool and developer. However, if there is a need to
+silent some diagnostics for a line of code, the ``NOLINT`` or ``NOLINTNEXTLINE``
+comments can be used. For example:
+
+.. code-block:: c++
+
+  class Foo
+  {
+// Skip all the diagnostics for the line
+Foo(int param); // NOLINT
+
+// Skip only the specified checks for the line
+Foo(double param); // NOLINT(google-explicit-constructor, google-runtime-int)
+
+// Skip only the specified diagnostics for the next line
+// NOLINTNEXTLINE (google-explicit-constructor, google-runtime-int)
+Foo(bool param); 
+  };
+
 .. _LibTooling: http://clang.llvm.org/docs/LibTooling.html
 .. _How To Setup Tooling For LLVM: http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -256,6 +256,9 @@
   - `hicpp-use-nullptr `_
   - `hicpp-vararg `_
 
+- The ``NOLINT`` and ``NOLINTNEXTLINE`` suppression comments were extended to support the list of checks
+  to disable in parentheses.
+
 Improvements to include-fixer
 -
 
Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -13,7 +13,18 @@
 
 class B { B(int i); }; // NOLINT
 
-class C { C(int i); }; // NOLINT(we-dont-care-about-categories-yet)
+class C { C(int i); }; // NOLINT(for-some-other-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
+
+class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
 
 void f() {
   int i;
@@ -35,4 +46,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -4,8 +4,24 @@
 // NOLINTNEXTLINE
 class B { B(int i); };
 
-// NOLINTNEXTLINE(we-dont-care-about-categories-yet)
+// NOLINTNEXTLINE(for-some-other-check)
 class C { C(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-check, google-explicit-constructor)
+class C4 { C4(int i); };
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };
 
 
 // NOLINTNEXTLINE
@@ -28,6 +44,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 4 warnings (4 NOLINT)
+// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include 
 #include 
@@ -290,7 +291,38 @@
   LastErrorPassesLineFilter = false;
 }
 
-static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc) {
+static bool IsNOLINTFound(StringRef NolintMacro, StringRef Line,
+  unsigned DiagID, const ClangTidyContext &Context) {
+  const size_t NolintIndex = Line.find(NolintMacro);
+  if (NolintIndex != StringRef::n

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 125258.
xgsa added a comment.

Release note item was reworded


https://reviews.llvm.org/D40671

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -250,6 +250,27 @@
   value:   'some value'
   ...
 
+Generally, there is no need to suppress :program:`clang-tidy` diagnostics. If
+there are false positives, either a bug should be reported or the code should be
+updated to be clear for both tool and developer. However, if there is a need to
+silent some diagnostics for a line of code, the ``NOLINT`` or ``NOLINTNEXTLINE``
+comments can be used. For example:
+
+.. code-block:: c++
+
+  class Foo
+  {
+// Skip all the diagnostics for the line
+Foo(int param); // NOLINT
+
+// Skip only the specified checks for the line
+Foo(double param); // NOLINT(google-explicit-constructor, google-runtime-int)
+
+// Skip only the specified diagnostics for the next line
+// NOLINTNEXTLINE (google-explicit-constructor, google-runtime-int)
+Foo(bool param); 
+  };
+
 .. _LibTooling: http://clang.llvm.org/docs/LibTooling.html
 .. _How To Setup Tooling For LLVM: http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -256,6 +256,9 @@
   - `hicpp-use-nullptr `_
   - `hicpp-vararg `_
 
+- Added an ability to specify in parentheses the list of checks to suppress for the ``NOLINT`` 
+  and ``NOLINTNEXTLINE`` comments.
+
 Improvements to include-fixer
 -
 
Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -13,7 +13,18 @@
 
 class B { B(int i); }; // NOLINT
 
-class C { C(int i); }; // NOLINT(we-dont-care-about-categories-yet)
+class C { C(int i); }; // NOLINT(for-some-other-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
+
+class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
 
 void f() {
   int i;
@@ -35,4 +46,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -4,8 +4,24 @@
 // NOLINTNEXTLINE
 class B { B(int i); };
 
-// NOLINTNEXTLINE(we-dont-care-about-categories-yet)
+// NOLINTNEXTLINE(for-some-other-check)
 class C { C(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-check, google-explicit-constructor)
+class C4 { C4(int i); };
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };
 
 
 // NOLINTNEXTLINE
@@ -28,6 +44,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 4 warnings (4 NOLINT)
+// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include 
 #include 
@@ -290,7 +291,38 @@
   LastErrorPassesLineFilter = false;
 }
 
-static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc) {
+static bool IsNOLINTFound(StringRef NolintMacro, StringRef Line,
+  unsigned DiagID, const ClangTidyContext &Context) {
+  const size_t NolintIndex = Line.find(NolintMacro);
+  if (NolintIndex != StringRef::npos) {
+size_t BracketIndex = Noli

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa added inline comments.



Comment at: test/clang-tidy/nolintnextline.cpp:23
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };

JonasToth wrote:
> Ian confused now. The NOLINTNEXTLINE with incorrect parents should not 
> silence the diagnostic, should it? 
> 
> In my understanding the following line should cause the explicit constructor 
> check to warn. Is that check message missing or did I get something wrong?
Without parentheses, it works just as `NOLINTNEXTLINE` (i.e. suppresses all the 
diagnostics for line), because it's impossible to distinguish check names from 
user comments after `NOLINTNEXTLINE`:
```
// NOLINTNEXTLINE check-name, another-check
// NOLINTNEXTLINE Some description, why the suppression is added
```


https://reviews.llvm.org/D40671



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

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa marked 2 inline comments as done.
xgsa added inline comments.



Comment at: docs/clang-tidy/index.rst:270
+// Skip only the specified diagnostics for the next line
+// NOLINTNEXTLINE (google-explicit-constructor, google-runtime-int)
+Foo(bool param); 

aaron.ballman wrote:
> Is the space before the `(` intended?
No, it's a mistake, thank you.


https://reviews.llvm.org/D40671



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

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 125291.
xgsa added a comment.

Updated documentation and comments in code.


https://reviews.llvm.org/D40671

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -250,6 +250,51 @@
   value:   'some value'
   ...
 
+While :program:`clang-tidy` diagnostics are intended to call out code that does
+not adhere to a coding standard, or is otherwise problematic in some way, there
+are times when it is more appropriate to silence the diagnostic instead of 
+changing the semantics of the code. In such circumstances, the ``NOLINT`` or
+``NOLINTNEXTLINE`` comments can be used to silence the diagnostic. For example:
+
+.. code-block:: c++
+
+  class Foo
+  {
+// Skip all the diagnostics for the line
+Foo(int param); // NOLINT
+
+// Skip only the specified checks for the line
+Foo(double param); // NOLINT(google-explicit-constructor, google-runtime-int)
+
+// Skip only the specified diagnostics for the next line
+// NOLINTNEXTLINE(google-explicit-constructor, google-runtime-int)
+Foo(bool param); 
+  };
+
+The formal syntax of ``NOLINT``/``NOLINTNEXTLINE`` is the following:
+
+.. parsed-literal::
+
+  lint-comment:
+lint-command
+lint-command lint-args
+
+  lint-args:
+**(** check-name-list **)**
+
+  check-name-list:
+*check-name*
+check-name-list **,** *check-name*
+
+  lint-command:
+**NOLINT**
+**NOLINTNEXTLINE**
+
+Note that whitespaces between ``NOLINT``/``NOLINTNEXTLINE`` and the opening
+parenthesis are not allowed (in this case the comment will be threated just as
+``NOLINT``/``NOLINTNEXTLINE``), whereas in check names list (inside
+the parenthesis) whitespaces can be used and will be ignored.
+
 .. _LibTooling: http://clang.llvm.org/docs/LibTooling.html
 .. _How To Setup Tooling For LLVM: http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -256,6 +256,8 @@
   - `hicpp-use-nullptr `_
   - `hicpp-vararg `_
 
+- Added the ability to suppress specific checks (or all checks) in a ``NOLINT`` or ``NOLINTNEXTLINE`` comment.
+
 Improvements to include-fixer
 -
 
Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -13,7 +13,18 @@
 
 class B { B(int i); }; // NOLINT
 
-class C { C(int i); }; // NOLINT(we-dont-care-about-categories-yet)
+class C { C(int i); }; // NOLINT(for-some-other-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
+
+class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
 
 void f() {
   int i;
@@ -35,4 +46,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -4,8 +4,24 @@
 // NOLINTNEXTLINE
 class B { B(int i); };
 
-// NOLINTNEXTLINE(we-dont-care-about-categories-yet)
+// NOLINTNEXTLINE(for-some-other-check)
 class C { C(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-check, google-explicit-constructor)
+class C4 { C4(int i); };
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };
 
 
 // NOLINTNEXTLINE
@@ -28,6 +44,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 4 warnings (4 NOLINT)
+// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagn

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 125292.
xgsa added a comment.

A typo in documentation was fixed.


https://reviews.llvm.org/D40671

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -250,6 +250,51 @@
   value:   'some value'
   ...
 
+While :program:`clang-tidy` diagnostics are intended to call out code that does
+not adhere to a coding standard, or is otherwise problematic in some way, there
+are times when it is more appropriate to silence the diagnostic instead of 
+changing the semantics of the code. In such circumstances, the ``NOLINT`` or
+``NOLINTNEXTLINE`` comments can be used to silence the diagnostic. For example:
+
+.. code-block:: c++
+
+  class Foo
+  {
+// Skip all the diagnostics for the line
+Foo(int param); // NOLINT
+
+// Skip only the specified checks for the line
+Foo(double param); // NOLINT(google-explicit-constructor, google-runtime-int)
+
+// Skip only the specified diagnostics for the next line
+// NOLINTNEXTLINE(google-explicit-constructor, google-runtime-int)
+Foo(bool param); 
+  };
+
+The formal syntax of ``NOLINT``/``NOLINTNEXTLINE`` is the following:
+
+.. parsed-literal::
+
+  lint-comment:
+lint-command
+lint-command lint-args
+
+  lint-args:
+**(** check-name-list **)**
+
+  check-name-list:
+*check-name*
+check-name-list **,** *check-name*
+
+  lint-command:
+**NOLINT**
+**NOLINTNEXTLINE**
+
+Note that whitespaces between ``NOLINT``/``NOLINTNEXTLINE`` and the opening
+parenthesis are not allowed (in this case the comment will be treated just as
+``NOLINT``/``NOLINTNEXTLINE``), whereas in check names list (inside
+the parenthesis) whitespaces can be used and will be ignored.
+
 .. _LibTooling: http://clang.llvm.org/docs/LibTooling.html
 .. _How To Setup Tooling For LLVM: http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -256,6 +256,8 @@
   - `hicpp-use-nullptr `_
   - `hicpp-vararg `_
 
+- Added the ability to suppress specific checks (or all checks) in a ``NOLINT`` or ``NOLINTNEXTLINE`` comment.
+
 Improvements to include-fixer
 -
 
Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -13,7 +13,18 @@
 
 class B { B(int i); }; // NOLINT
 
-class C { C(int i); }; // NOLINT(we-dont-care-about-categories-yet)
+class C { C(int i); }; // NOLINT(for-some-other-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
+
+class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
 
 void f() {
   int i;
@@ -35,4 +46,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -4,8 +4,24 @@
 // NOLINTNEXTLINE
 class B { B(int i); };
 
-// NOLINTNEXTLINE(we-dont-care-about-categories-yet)
+// NOLINTNEXTLINE(for-some-other-check)
 class C { C(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-check, google-explicit-constructor)
+class C4 { C4(int i); };
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };
 
 
 // NOLINTNEXTLINE
@@ -28,6 +44,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 4 warnings (4 NOLINT)
+// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsu

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa added inline comments.



Comment at: docs/clang-tidy/index.rst:280
+lint-command
+lint-command lint-args
+

aaron.ballman wrote:
> This should have a subscript `opt` following `lint-args` to denote that the 
> args are optional. I think you can achieve that with:
> ```
> lint-args\ :sub:`opt`
> ```
Sorry, I am not very familiar with Sphinx, but
```
\ :sub:`opt`
```
seems doesn't work inside `parsed-literal` block (I have searched through the 
llvm docs and found a few similar places, which uses such construction in 
`parsed-literal` block, and it is not rendered properly even on official web 
site).
Here `parsed-literal` block is used to render a quote-like block with custom 
formatting, so the whole grammar is shown as a single entity, and I cannot 
achieve this with the other constructs. 
To show that `lint-args` is optional I split this grammar rule into 2:
```
lint-command
lint-command lint-args
```

Isn't it enough or are there any suggestions how to handle this?


https://reviews.llvm.org/D40671



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

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa marked 2 inline comments as done.
xgsa added inline comments.



Comment at: docs/clang-tidy/index.rst:253
 
+Generally, there is no need to suppress :program:`clang-tidy` diagnostics. If
+there are false positives, either a bug should be reported or the code should 
be

alexfh wrote:
> aaron.ballman wrote:
> > I don't agree with that initial statement's use of "generally" -- checks 
> > that are chatty live in clang-tidy, as are checks for various coding 
> > standards (which commonly have a  deviation mechanism). Also, I don't think 
> > we should encourage users to unconditionally report false positives as 
> > bugs; many of the coding standard related checks provide true positives 
> > that are annoying and users may want to deviate in certain circumstances 
> > (like CERT's rule banning use of `rand()` or `system()`). I would reword 
> > this to:
> > ```
> > While clang-tidy diagnostics are intended to call out code that does not 
> > adhere to a coding standard, or is otherwise problematic in some way, there 
> > are times when it is more appropriate to silence the diagnostic instead of 
> > changing the semantics of the code. In such circumstances, the NOLINT or 
> > NOLINTNEXTLINE comments can be used to silence the diagnostic. For example:
> > ```
> > I would also describe the comment syntax more formally as (my markdown may 
> > be incorrect, you should ensure this renders sensibly), with surrounding 
> > prose:
> > ```
> > *lint-comment:*
> >   *lint-command* *lint-args~opt~*
> >   
> > *lint-args:*
> >   `(` *check-name-list* `)`
> > 
> > *check-name-list:*
> >   *check-name*
> >   *check-name-list* `,` *check-name*
> > 
> > *lint-command:*
> >   `NOLINT`
> >   `NOLINTNEXTLINE`
> > ```
> > Specific to the prose mentioned above, you should document where the 
> > feature is tolerant to whitespace (can there be a space between NOLINT and 
> > the parens, what about inside the parens, how about after or before commas, 
> > etc).
> One little request from me: the documentation should recommend using 
> check-specific ways to mute diagnostics, if they are available (e.g. 
> bugprone-use-after-move can be silenced by re-initializing the variable after 
> it has been moved out, misc-string-integer-assignment can be suppressed by 
> explicitly casting the integer to char, readability-implicit-bool-conversion 
> can also be suppressed by using explicit casts, etc.). NOLINT(NEXTLINE)? 
> should be treated as the last resort, when fixing the code is infeasible or 
> impractical and there's no diagnostic-specific suppression mechanism 
> available.
Thank you for the examples of the check-specific mute ways, I have looked for 
that, but haven't found the good ones.


https://reviews.llvm.org/D40671



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

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 125328.

https://reviews.llvm.org/D40671

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -250,6 +250,57 @@
   value:   'some value'
   ...
 
+While :program:`clang-tidy` diagnostics are intended to call out code that does
+not adhere to a coding standard, or is otherwise problematic in some way, there
+are times when it is more appropriate to silence the diagnostic instead of 
+changing the semantics of the code. The preferable way of doing this is using
+the check-specific ways to mute diagnostics, if they are available (e.g. 
+bugprone-use-after-move can be silenced by re-initializing the variable after
+it has been moved out, misc-string-integer-assignment can be suppressed by
+explicitly casting the integer to char, readability-implicit-bool-conversion
+can also be suppressed by using explicit casts, etc.). Otherwise,
+the ``NOLINT`` or ``NOLINTNEXTLINE`` comments can be used to silence
+the diagnostic. For example:
+
+.. code-block:: c++
+
+  class Foo
+  {
+// Skip all the diagnostics for the line
+Foo(int param); // NOLINT
+
+// Skip only the specified checks for the line
+Foo(double param); // NOLINT(google-explicit-constructor, google-runtime-int)
+
+// Skip only the specified diagnostics for the next line
+// NOLINTNEXTLINE(google-explicit-constructor, google-runtime-int)
+Foo(bool param); 
+  };
+
+The formal syntax of ``NOLINT``/``NOLINTNEXTLINE`` is the following:
+
+.. parsed-literal::
+
+  lint-comment:
+lint-command
+lint-command lint-args
+
+  lint-args:
+**(** check-name-list **)**
+
+  check-name-list:
+*check-name*
+check-name-list **,** *check-name*
+
+  lint-command:
+**NOLINT**
+**NOLINTNEXTLINE**
+
+Note that whitespaces between ``NOLINT``/``NOLINTNEXTLINE`` and the opening
+parenthesis are not allowed (in this case the comment will be treated just as
+``NOLINT``/``NOLINTNEXTLINE``), whereas in check names list (inside
+the parenthesis) whitespaces can be used and will be ignored.
+
 .. _LibTooling: http://clang.llvm.org/docs/LibTooling.html
 .. _How To Setup Tooling For LLVM: http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -256,6 +256,8 @@
   - `hicpp-use-nullptr `_
   - `hicpp-vararg `_
 
+- Added the ability to suppress specific checks (or all checks) in a ``NOLINT`` or ``NOLINTNEXTLINE`` comment.
+
 Improvements to include-fixer
 -
 
Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -13,7 +13,18 @@
 
 class B { B(int i); }; // NOLINT
 
-class C { C(int i); }; // NOLINT(we-dont-care-about-categories-yet)
+class C { C(int i); }; // NOLINT(for-some-other-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
+
+class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
 
 void f() {
   int i;
@@ -35,4 +46,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -4,8 +4,24 @@
 // NOLINTNEXTLINE
 class B { B(int i); };
 
-// NOLINTNEXTLINE(we-dont-care-about-categories-yet)
+// NOLINTNEXTLINE(for-some-other-check)
 class C { C(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-check, google-explicit-constructor)
+class C4 { C4(int i); };
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };
 
 
 // NOLINTNEXTLINE
@@ -28,6 +44,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 4 warning

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa marked 2 inline comments as done.
xgsa added inline comments.



Comment at: docs/clang-tidy/index.rst:254-255
+While :program:`clang-tidy` diagnostics are intended to call out code that does
+not adhere to a coding standard, or is otherwise problematic in some way, there
+are times when it is more appropriate to silence the diagnostic instead of 
+changing the semantics of the code. The preferable way of doing this is using

aaron.ballman wrote:
> I would reword this somewhat now. I would put a period before ", there are 
> times" and then move that whole "there are times" clause below.
I tried to move the "there are times"-clause below, but in this case "The 
preferable way of doing this is using..." becomes unclear, because it tries to 
explain the way of doing something without naming the purpose that is expected 
to achieve. So I suppose, it is necessary to place "However, there are times 
when it is more appropriate to silence the diagnostic instead of changing the 
semantics of the code" here. Am I missing something?


https://reviews.llvm.org/D40671



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

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 125517.
xgsa added a comment.

Updated documentation


https://reviews.llvm.org/D40671

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -250,6 +250,56 @@
   value:   'some value'
   ...
 
+:program:`clang-tidy` diagnostics are intended to call out code that does
+not adhere to a coding standard, or is otherwise problematic in some way.
+However, there are times, when it is more appropriate to silence the diagnostic
+instead of changing the semantics of the code. The preferred way of doing this
+is using the check-specific ways, if they are available (e.g. 
+bugprone-use-after-move can be silenced by re-initializing the variable after 
+it has been moved out, misc-string-integer-assignment can be suppressed by 
+explicitly casting the integer to char, readability-implicit-bool-conversion
+can also be suppressed by using explicit casts, etc.). Otherwise, 
+the ``NOLINT`` or ``NOLINTNEXTLINE`` comments can be used. For example:
+
+.. code-block:: c++
+
+  class Foo
+  {
+// Silent all the diagnostics for the line
+Foo(int param); // NOLINT
+
+// Silent only the specified checks for the line
+Foo(double param); // NOLINT(google-explicit-constructor, google-runtime-int)
+
+// Silent only the specified diagnostics for the next line
+// NOLINTNEXTLINE(google-explicit-constructor, google-runtime-int)
+Foo(bool param); 
+  };
+
+The formal syntax of ``NOLINT``/``NOLINTNEXTLINE`` is the following:
+
+.. parsed-literal::
+
+  lint-comment:
+lint-command
+lint-command lint-args
+
+  lint-args:
+**(** check-name-list **)**
+
+  check-name-list:
+*check-name*
+check-name-list **,** *check-name*
+
+  lint-command:
+**NOLINT**
+**NOLINTNEXTLINE**
+
+Note that whitespaces between ``NOLINT``/``NOLINTNEXTLINE`` and the opening
+parenthesis are not allowed (in this case the comment will be treated just as
+``NOLINT``/``NOLINTNEXTLINE``), whereas in check names list (inside
+the parenthesis) whitespaces can be used and will be ignored.
+
 .. _LibTooling: http://clang.llvm.org/docs/LibTooling.html
 .. _How To Setup Tooling For LLVM: http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -256,6 +256,8 @@
   - `hicpp-use-nullptr `_
   - `hicpp-vararg `_
 
+- Added the ability to suppress specific checks (or all checks) in a ``NOLINT`` or ``NOLINTNEXTLINE`` comment.
+
 Improvements to include-fixer
 -
 
Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -13,7 +13,18 @@
 
 class B { B(int i); }; // NOLINT
 
-class C { C(int i); }; // NOLINT(we-dont-care-about-categories-yet)
+class C { C(int i); }; // NOLINT(for-some-other-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
+
+class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
 
 void f() {
   int i;
@@ -35,4 +46,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -4,8 +4,24 @@
 // NOLINTNEXTLINE
 class B { B(int i); };
 
-// NOLINTNEXTLINE(we-dont-care-about-categories-yet)
+// NOLINTNEXTLINE(for-some-other-check)
 class C { C(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-check, google-explicit-constructor)
+class C4 { C4(int i); };
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };
 
 
 // NOLINTNEXTLINE
@@ -28,6 +44,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa added a comment.

In https://reviews.llvm.org/D40671#944906, @alexfh wrote:

> BTW, how will this feature interact with cpplint.py's way of handling 
> specific NOLINT directives that use different lint rule names, which 
> sometimes refer to the same rule (e.g. `// NOLINT(runtime/explicit)` 
> suppresses the `runtime/explicit` cpplint rule that enforces the same style 
> rule as the google-runtime-explicit check)?


This feature accepts only full check names.


https://reviews.llvm.org/D40671



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

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa added a comment.

So can this patch be submitted? Should I do something to make it happen?


https://reviews.llvm.org/D40671



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

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa marked an inline comment as done.
xgsa added a comment.

In https://reviews.llvm.org/D40671#948826, @aaron.ballman wrote:

> There are still some outstanding concerns around the documentation wording, 
> but once those are resolved it should be ready to commit. If you don't have 
> commit access, I can commit on your behalf -- just let me know.


Yes, I don't have commit access, so I need your help with this, Aaron. Thank 
you!


https://reviews.llvm.org/D40671



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

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 126058.
xgsa added a comment.

Documentation update


https://reviews.llvm.org/D40671

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/nolint.cpp
  test/clang-tidy/nolintnextline.cpp

Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -250,6 +250,56 @@
   value:   'some value'
   ...
 
+:program:`clang-tidy` diagnostics are intended to call out code that does
+not adhere to a coding standard, or is otherwise problematic in some way.
+However, if it is known that the code is correct, the check-specific ways
+to silence the diagnostics could be used, if they are available (e.g. 
+bugprone-use-after-move can be silenced by re-initializing the variable after 
+it has been moved out, misc-string-integer-assignment can be suppressed by 
+explicitly casting the integer to char, readability-implicit-bool-conversion
+can also be suppressed by using explicit casts, etc.). If they are not 
+available or if changing the semantics of the code is not desired, 
+the ``NOLINT`` or ``NOLINTNEXTLINE`` comments can be used instead. For example:
+
+.. code-block:: c++
+
+  class Foo
+  {
+// Silent all the diagnostics for the line
+Foo(int param); // NOLINT
+
+// Silent only the specified checks for the line
+Foo(double param); // NOLINT(google-explicit-constructor, google-runtime-int)
+
+// Silent only the specified diagnostics for the next line
+// NOLINTNEXTLINE(google-explicit-constructor, google-runtime-int)
+Foo(bool param); 
+  };
+
+The formal syntax of ``NOLINT``/``NOLINTNEXTLINE`` is the following:
+
+.. parsed-literal::
+
+  lint-comment:
+lint-command
+lint-command lint-args
+
+  lint-args:
+**(** check-name-list **)**
+
+  check-name-list:
+*check-name*
+check-name-list **,** *check-name*
+
+  lint-command:
+**NOLINT**
+**NOLINTNEXTLINE**
+
+Note that whitespaces between ``NOLINT``/``NOLINTNEXTLINE`` and the opening
+parenthesis are not allowed (in this case the comment will be treated just as
+``NOLINT``/``NOLINTNEXTLINE``), whereas in check names list (inside
+the parenthesis) whitespaces can be used and will be ignored.
+
 .. _LibTooling: http://clang.llvm.org/docs/LibTooling.html
 .. _How To Setup Tooling For LLVM: http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -256,6 +256,8 @@
   - `hicpp-use-nullptr `_
   - `hicpp-vararg `_
 
+- Added the ability to suppress specific checks (or all checks) in a ``NOLINT`` or ``NOLINTNEXTLINE`` comment.
+
 Improvements to include-fixer
 -
 
Index: test/clang-tidy/nolint.cpp
===
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -13,7 +13,18 @@
 
 class B { B(int i); }; // NOLINT
 
-class C { C(int i); }; // NOLINT(we-dont-care-about-categories-yet)
+class C { C(int i); }; // NOLINT(for-some-other-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
+
+class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
 
 void f() {
   int i;
@@ -35,4 +46,4 @@
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
Index: test/clang-tidy/nolintnextline.cpp
===
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -4,8 +4,24 @@
 // NOLINTNEXTLINE
 class B { B(int i); };
 
-// NOLINTNEXTLINE(we-dont-care-about-categories-yet)
+// NOLINTNEXTLINE(for-some-other-check)
 class C { C(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-check, google-explicit-constructor)
+class C4 { C4(int i); };
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };
 
 
 // NOLINTNEXTLINE
@@ -28,6 +44,6 @@
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppres

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa added a comment.

In https://reviews.llvm.org/D40671#949687, @alexfh wrote:

> How are unknown check names handled? More specifically: will the `// 
> NOLINT(runtime/explicit)` comment disable all clang-tidy checks or none?


None. If comment is syntactically correct and contains parenthesis, it works 
only for the known checks inside.

Still, I don't think it worth mentioning all the corner cases in documentation 
to keep things simple.


https://reviews.llvm.org/D40671



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

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa added a comment.

@aaron.ballman, sorry for my insistence, but it seems all the comments are 
fixed and the patch is ready for commit or am I missing something? Could you 
please commit it on my behalf, as I don't have rights to do that?


https://reviews.llvm.org/D40671



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

[PATCH] D39622: Fix type name generation in DWARF for template instantiations with enum types and template specializations

[Anton via Phabricator via cfe-commits]

xgsa updated this revision to Diff 126826.
xgsa retitled this revision from "Fix type debug information generation for 
enum-based template specialization" to "Fix type name generation in DWARF for 
template instantiations with enum types and template specializations".
xgsa edited the summary of this revision.
xgsa added a comment.
Herald added a subscriber: JDevlieghere.

One more case was handled, review comments were applied, but no tests though, 
because I still not sure if the approach I have chosen is correct.


https://reviews.llvm.org/D39622

Files:
  include/clang/AST/PrettyPrinter.h
  lib/AST/TemplateBase.cpp
  lib/AST/TypePrinter.cpp
  lib/CodeGen/CGDebugInfo.cpp


Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -234,6 +234,7 @@
   if (CGM.getCodeGenOpts().EmitCodeView)
 PP.MSVCFormatting = true;
 
+  PP.ABICompatibleFormatting = true;
   return PP;
 }
 
Index: lib/AST/TypePrinter.cpp
===
--- lib/AST/TypePrinter.cpp
+++ lib/AST/TypePrinter.cpp
@@ -1060,7 +1060,9 @@
   if (ClassTemplateSpecializationDecl *Spec
 = dyn_cast(D)) {
 ArrayRef Args;
-if (TypeSourceInfo *TAW = Spec->getTypeAsWritten()) {
+if (TypeSourceInfo *TAW = !Policy.ABICompatibleFormatting
+  ? Spec->getTypeAsWritten()
+  : nullptr) {
   const TemplateSpecializationType *TST =
 cast(TAW->getType());
   Args = TST->template_arguments();
Index: lib/AST/TemplateBase.cpp
===
--- lib/AST/TemplateBase.cpp
+++ lib/AST/TemplateBase.cpp
@@ -56,14 +56,22 @@
   const llvm::APSInt &Val = TemplArg.getAsIntegral();
 
   if (const EnumType *ET = T->getAs()) {
-for (const EnumConstantDecl* ECD : ET->getDecl()->enumerators()) {
-  // In Sema::CheckTemplateArugment, enum template arguments value are
-  // extended to the size of the integer underlying the enum type.  This
-  // may create a size difference between the enum value and template
-  // argument value, requiring isSameValue here instead of operator==.
-  if (llvm::APSInt::isSameValue(ECD->getInitVal(), Val)) {
-ECD->printQualifiedName(Out, Policy);
-return;
+if (Policy.ABICompatibleFormatting) {
+  Out << "(";
+  ET->getDecl()->getNameForDiagnostic(Out, Policy, true);
+  Out << ")";
+  Out << Val;
+  return;
+} else {
+  for (const EnumConstantDecl* ECD : ET->getDecl()->enumerators()) {
+// In Sema::CheckTemplateArugment, enum template arguments value are
+// extended to the size of the integer underlying the enum type.  This
+// may create a size difference between the enum value and template
+// argument value, requiring isSameValue here instead of operator==.
+if (llvm::APSInt::isSameValue(ECD->getInitVal(), Val)) {
+  ECD->printQualifiedName(Out, Policy);
+  return;
+}
   }
 }
   }
Index: include/clang/AST/PrettyPrinter.h
===
--- include/clang/AST/PrettyPrinter.h
+++ include/clang/AST/PrettyPrinter.h
@@ -52,7 +52,7 @@
   Half(LO.Half), MSWChar(LO.MicrosoftExt && !LO.WChar),
   IncludeNewlines(true), MSVCFormatting(false),
   ConstantsAsWritten(false), SuppressImplicitBase(false),
-  FullyQualifiedName(false) { }
+  FullyQualifiedName(false), ABICompatibleFormatting(false) { }
 
   /// Adjust this printing policy for cases where it's known that we're
   /// printing C++ code (for instance, if AST dumping reaches a C++-only
@@ -225,6 +225,13 @@
   /// When true, print the fully qualified name of function declarations.
   /// This is the opposite of SuppressScope and thus overrules it.
   bool FullyQualifiedName : 1;
+
+  /// Use formatting compatible with ABI specification. It is necessary for
+  /// saving entities into debug tables which have to be compatible with
+  /// the representation, described in ABI specification. In particular, this 
forces
+  /// templates parametrized with enums to be represented as "T<(Enum)0>" 
instead of
+  /// "T" and template specializations to be written in canonical 
form.
+  bool ABICompatibleFormatting : 1;
 };
 
 } // end namespace clang


Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -234,6 +234,7 @@
   if (CGM.getCodeGenOpts().EmitCodeView)
 PP.MSVCFormatting = true;
 
+  PP.ABICompatibleFormatting = true;
   return PP;
 }
 
Index: lib/AST/TypePrinter.cpp
===
--- lib/AST/TypePrinter.cpp
+++ lib/AST/TypePrinter.cpp
@@ -1060,7 +1060,9 @@
   if (ClassTemplateSpecializationDecl *Spec
 

[PATCH] D39622: Fix type debug information generation for enum-based template specialization

[Anton via Phabricator via cfe-commits]

xgsa created this revision.
xgsa added a project: clang.
Herald added a subscriber: aprantl.

Currently, there is an inconsistency between type name record in ".apple_types" 
section and entry in symbols table for the same type. In particular, for such 
types lldb is unable to resolve the real type using RTTI. Consider an example:

  enum class TagType : bool
  {
  Tag1
  };
  
  class I
  {
  };
  
  template 
  class Impl : public I
  {
  private:
  int v = 123;
  };
  
  int main(int argc, const char * argv[]) {
  Impl impl;
  I& i = impl;
  return 0;  // [*]
  }

For such code clang generates class name "Impl" into 
".apple_types" and "Impl<(TagType)0>" into symbols table. Thus at point [*], 
the lldb is unable to resolve the real type of the "i" variable (shows the type 
as "I&"), whereas it is expected to be "Impl" or 
"Impl<(TagType)0>".

This patch fixes the issue. I haven't found the existing ticket in bugzilla for 
the described issue (https://bugs.llvm.org/show_bug.cgi?id=24198 -- this one 
looks similar, but it's not the same and actually it is not reproducible 
anymore).


Repository:
  rL LLVM

https://reviews.llvm.org/D39622

Files:
  include/clang/AST/PrettyPrinter.h
  lib/AST/TemplateBase.cpp
  lib/CodeGen/CGDebugInfo.cpp


Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -234,6 +234,7 @@
   if (CGM.getCodeGenOpts().EmitCodeView)
 PP.MSVCFormatting = true;
 
+  PP.ABICompatibleFormatting = true;
   return PP;
 }
 
Index: lib/AST/TemplateBase.cpp
===
--- lib/AST/TemplateBase.cpp
+++ lib/AST/TemplateBase.cpp
@@ -41,14 +41,22 @@
   const llvm::APSInt &Val = TemplArg.getAsIntegral();
 
   if (const EnumType *ET = T->getAs()) {
-for (const EnumConstantDecl* ECD : ET->getDecl()->enumerators()) {
-  // In Sema::CheckTemplateArugment, enum template arguments value are
-  // extended to the size of the integer underlying the enum type.  This
-  // may create a size difference between the enum value and template
-  // argument value, requiring isSameValue here instead of operator==.
-  if (llvm::APSInt::isSameValue(ECD->getInitVal(), Val)) {
-ECD->printQualifiedName(Out, Policy);
-return;
+if (Policy.ABICompatibleFormatting) {
+  Out << "(";
+  ET->getDecl()->getNameForDiagnostic(Out, Policy, true);
+  Out << ")";
+  Out << Val;
+  return;
+} else {
+  for (const EnumConstantDecl* ECD : ET->getDecl()->enumerators()) {
+// In Sema::CheckTemplateArugment, enum template arguments value are
+// extended to the size of the integer underlying the enum type.  This
+// may create a size difference between the enum value and template
+// argument value, requiring isSameValue here instead of operator==.
+if (llvm::APSInt::isSameValue(ECD->getInitVal(), Val)) {
+  ECD->printQualifiedName(Out, Policy);
+  return;
+}
   }
 }
   }
Index: include/clang/AST/PrettyPrinter.h
===
--- include/clang/AST/PrettyPrinter.h
+++ include/clang/AST/PrettyPrinter.h
@@ -51,7 +51,8 @@
   TerseOutput(false), PolishForDeclaration(false),
   Half(LO.Half), MSWChar(LO.MicrosoftExt && !LO.WChar),
   IncludeNewlines(true), MSVCFormatting(false),
-  ConstantsAsWritten(false), SuppressImplicitBase(false) { }
+  ConstantsAsWritten(false), SuppressImplicitBase(false),
+  ABICompatibleFormatting(false) { }
 
   /// \brief Adjust this printing policy for cases where it's known that
   /// we're printing C++ code (for instance, if AST dumping reaches a
@@ -64,7 +65,7 @@
   }
 
   /// \brief The number of spaces to use to indent each line.
-  unsigned Indentation : 8;
+  unsigned Indentation : 7;
 
   /// \brief Whether we should suppress printing of the actual specifiers for
   /// the given type or declaration.
@@ -222,6 +223,13 @@
 
   /// \brief When true, don't print the implicit 'self' or 'this' expressions.
   bool SuppressImplicitBase : 1;
+
+  /// \brief Use formatting compatible with ABI specification. It is necessary 
for
+  /// saving entities into debug tables which have to be compatible with
+  /// the representation, described in ABI specification. In particular, this 
forces
+  /// templates parametrized with enums to be represented as "T<(Enum)0>" 
instead of
+  /// "T".
+  bool ABICompatibleFormatting : 1;
 };
 
 } // end namespace clang


Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -234,6 +234,7 @@
   if (CGM.getCodeGenOpts().EmitCodeView)
 PP.MSVCFormatting = true;
 
+  PP.ABICompatibleFormatting = true;
   return PP;
 }
 
Index: lib/AST/TemplateBase.cpp
==

[PATCH] D39622: Fix type debug information generation for enum-based template specialization

[Anton via Phabricator via cfe-commits]

xgsa added a comment.

In https://reviews.llvm.org/D39622#915722, @aprantl wrote:

> Can you add a testcase?


Definitely, I just wanted to know if such fix is correct at all. Moreover, 
could you please help me with the correct place for a test case? Possibly, 
there are some similar test case I can look at, it would be very helpful.


Repository:
  rL LLVM

https://reviews.llvm.org/D39622



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

[PATCH] D39622: Fix type debug information generation for enum-based template specialization

[Anton via Phabricator via cfe-commits]

xgsa added inline comments.



Comment at: include/clang/AST/PrettyPrinter.h:68
   /// \brief The number of spaces to use to indent each line.
-  unsigned Indentation : 8;
+  unsigned Indentation : 7;
 

aprantl wrote:
> this change looks like it has the potential to break existing code.
If not to change the size of this field, the overall size of the PrintingPolicy 
will exceed 32 bits, so it won't fit a CPU register on 32-bit systems and will 
be less lightweight. Is it OK, should this line to be reverted?



Comment at: include/clang/AST/PrettyPrinter.h:227
+
+  /// \brief Use formatting compatible with ABI specification. It is necessary 
for
+  /// saving entities into debug tables which have to be compatible with

aprantl wrote:
> Te \brief is redundant and can be omitted.
I reviewed the other descriptions once again and I suppose it would be more 
consistent to have "\brief Use formatting compatible with ABI specification." 
and the rest of the description as a separate paragraph. Don't you mind against 
such fix?


Repository:
  rL LLVM

https://reviews.llvm.org/D39622



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

[PATCH] D39633: Remove \brief from doxygen comments in PrettyPrinter.h

[Anton via Phabricator via cfe-commits]

xgsa created this revision.
xgsa added a project: clang.

For consistency with the code proposed in https://reviews.llvm.org/D39622


Repository:
  rL LLVM

https://reviews.llvm.org/D39633

Files:
  include/clang/AST/PrettyPrinter.h

Index: include/clang/AST/PrettyPrinter.h
===
--- include/clang/AST/PrettyPrinter.h
+++ include/clang/AST/PrettyPrinter.h
@@ -30,8 +30,8 @@
   virtual bool handledStmt(Stmt* E, raw_ostream& OS) = 0;
 };
 
-/// \brief Describes how types, statements, expressions, and
-/// declarations should be printed.
+/// Describes how types, statements, expressions, and declarations should be
+/// printed.
 ///
 /// This type is intended to be small and suitable for passing by value.
 /// It is very frequently copied.
@@ -53,10 +53,10 @@
   IncludeNewlines(true), MSVCFormatting(false),
   ConstantsAsWritten(false), SuppressImplicitBase(false) { }
 
-  /// \brief Adjust this printing policy for cases where it's known that
-  /// we're printing C++ code (for instance, if AST dumping reaches a
-  /// C++-only construct). This should not be used if a real LangOptions
-  /// object is available.
+  /// Adjust this printing policy for cases where it's known that we're
+  /// printing C++ code (for instance, if AST dumping reaches a C++-only
+  /// construct). This should not be used if a real LangOptions object is
+  /// available.
   void adjustForCPlusPlus() {
 SuppressTagKeyword = true;
 Bool = true;
@@ -63,10 +63,10 @@
 UseVoidForZeroParams = false;
   }
 
-  /// \brief The number of spaces to use to indent each line.
+  /// The number of spaces to use to indent each line.
   unsigned Indentation : 8;
 
-  /// \brief Whether we should suppress printing of the actual specifiers for
+  /// Whether we should suppress printing of the actual specifiers for
   /// the given type or declaration.
   ///
   /// This flag is only used when we are printing declarators beyond
@@ -82,7 +82,7 @@
   /// "const int" type specifier and instead only print the "*y".
   bool SuppressSpecifiers : 1;
 
-  /// \brief Whether type printing should skip printing the tag keyword.
+  /// Whether type printing should skip printing the tag keyword.
   ///
   /// This is used when printing the inner type of elaborated types,
   /// (as the tag keyword is part of the elaborated type):
@@ -92,7 +92,7 @@
   /// \endcode
   bool SuppressTagKeyword : 1;
 
-  /// \brief When true, include the body of a tag definition.
+  /// When true, include the body of a tag definition.
   ///
   /// This is used to place the definition of a struct
   /// in the middle of another declaration as with:
@@ -102,14 +102,14 @@
   /// \endcode
   bool IncludeTagDefinition : 1;
 
-  /// \brief Suppresses printing of scope specifiers.
+  /// Suppresses printing of scope specifiers.
   bool SuppressScope : 1;
 
-  /// \brief Suppress printing parts of scope specifiers that don't need
+  /// Suppress printing parts of scope specifiers that don't need
   /// to be written, e.g., for inline or anonymous namespaces.
   bool SuppressUnwrittenScope : 1;
   
-  /// \brief Suppress printing of variable initializers.
+  /// Suppress printing of variable initializers.
   ///
   /// This flag is used when printing the loop variable in a for-range
   /// statement. For example, given:
@@ -122,8 +122,8 @@
   /// internal initializer constructed for x will not be printed.
   bool SuppressInitializers : 1;
 
-  /// \brief Whether we should print the sizes of constant array expressions
-  /// as written in the sources.
+  /// Whether we should print the sizes of constant array expressions as written
+  /// in the sources.
   ///
   /// This flag determines whether array types declared as
   ///
@@ -140,17 +140,15 @@
   /// \endcode
   bool ConstantArraySizeAsWritten : 1;
   
-  /// \brief When printing an anonymous tag name, also print the location of
-  /// that entity (e.g., "enum "). Otherwise, just 
-  /// prints "(anonymous)" for the name.
+  /// When printing an anonymous tag name, also print the location of that
+  /// entity (e.g., "enum "). Otherwise, just prints
+  /// "(anonymous)" for the name.
   bool AnonymousTagLocations : 1;
   
-  /// \brief When true, suppress printing of the __strong lifetime qualifier in
-  /// ARC.
+  /// When true, suppress printing of the __strong lifetime qualifier in ARC.
   unsigned SuppressStrongLifetime : 1;
   
-  /// \brief When true, suppress printing of lifetime qualifier in
-  /// ARC.
+  /// When true, suppress printing of lifetime qualifier in ARC.
   unsigned SuppressLifetimeQualifiers : 1;
 
   /// When true, suppresses printing template arguments in names of C++
@@ -157,24 +155,24 @@
   /// constructors.
   unsigned SuppressTemplateArgsInCXXConstructors : 1;
 
-  /// \brief Whether we can use 'bool' rather than '_Bool' (even if the language
+  /// Whether we can use 'bool' rather than '_Bool' (even if the language
   /// doesn't ac

[PATCH] D39622: Fix type debug information generation for enum-based template specialization

[Anton via Phabricator via cfe-commits]

xgsa added inline comments.



Comment at: include/clang/AST/PrettyPrinter.h:227
+
+  /// \brief Use formatting compatible with ABI specification. It is necessary 
for
+  /// saving entities into debug tables which have to be compatible with

aprantl wrote:
> xgsa wrote:
> > aprantl wrote:
> > > Te \brief is redundant and can be omitted.
> > I reviewed the other descriptions once again and I suppose it would be more 
> > consistent to have "\brief Use formatting compatible with ABI 
> > specification." and the rest of the description as a separate paragraph. 
> > Don't you mind against such fix?
> This is obviously not super important, but since you asked: our policy for 
> how we use Doxygen has evolved over time, and this file represents an older 
> version of that policy. The right thing to do would be to first commit a 
> cleanup patch that removes all existing `\brief` occurrences from the file 
> and then land the patch.
Thank you for clarification, I don't mind doing the right thing: 
https://reviews.llvm.org/D39633

Still, I don't have rights to commit the patch, so somebody should commit it 
anyway.


Repository:
  rL LLVM

https://reviews.llvm.org/D39622



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

[PATCH] D39622: Fix type debug information generation for enum-based template specialization

[Anton via Phabricator via cfe-commits]

xgsa added a comment.

In https://reviews.llvm.org/D39622#919579, @aprantl wrote:

> For clarification: what is the "symbols table" you are referring to in the 
> description?


I meant the data dumped with "nm -an ./test".

By the way, I haven't abandoned the patch, I have found one more case when my 
fix doesn't work and I am working on improvement. Nevertheless, it would be 
helpful to get answers to the questions in this review (about changing the 
"Indentation" field and about the test).


Repository:
  rL LLVM

https://reviews.llvm.org/D39622



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