[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive
alexfh added a comment. In https://reviews.llvm.org/D40671#954906, @xgsa wrote: > 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? The existing clang-tidy infrastructure for checks doesn't provide a way for checks to interact or to collect other checks' diagnostics, and I'm not sure this functionality would be helpful beyond this specific feature. So all of this seems more reasonable to implement as a part of clang-tidy itself. 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
aaron.ballman closed this revision. aaron.ballman added a comment. Committed in r320713. 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
aaron.ballman added a comment. In https://reviews.llvm.org/D40671#954906, @xgsa wrote: > 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? I think this is desirable functionality and can be done in follow-up patches. NOLINT of unknown check names should be pretty easy, but detecting NOLINT comments on lines that do not trigger the specified diagnostic may be a bit more tricky. 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
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] D40671: [clang-tidy] Support specific checks for NOLINT directive
alexfh accepted this revision. alexfh added a comment. In https://reviews.llvm.org/D40671#949732, @xgsa wrote: > 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. Documenting interaction with cpplint-style NOLINT categories would potentially save time to the users and clang-tidy maintainers (at least for codebases using Google style and cpplint). Fine for a follow-up. In https://reviews.llvm.org/D40671#953888, @aaron.ballman wrote: > In https://reviews.llvm.org/D40671#953291, @xgsa wrote: > > > @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? > > > The check now LGTM, but I am going to wait to commit in case @alexfh has > concerns regarding unknown check names. > > 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. 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
aaron.ballman added a comment. In https://reviews.llvm.org/D40671#953291, @xgsa wrote: > @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? The check now LGTM, but I am going to wait to commit in case @alexfh has concerns regarding unknown check names. 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. 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
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] D40671: [clang-tidy] Support specific checks for NOLINT directive
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
alexfh added a comment. In https://reviews.llvm.org/D40671#945158, @xgsa wrote: > 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. How are unknown check names handled? More specifically: will the `// NOLINT(runtime/explicit)` comment disable all clang-tidy checks or none? 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
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:
[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive
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
aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. In https://reviews.llvm.org/D40671#947762, @xgsa wrote: > So can this patch be submitted? Should I do something to make it happen? 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. 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 xgsa wrote: > 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? The current formatting of this flows strangely. It starts by saying clang-tidy calls out problems, says there are times when it is more appropriate to silence the diagnostic without changing the code, then says the preferred way is to change the code, then says you can NOLINT it if you don't want to change the code. I'd prefer if the flow was: clang-tidy calls out problems, the preferred way is to change the code, but there are still times when changing the code is not helpful and thus you can use NOLINT. 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
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
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
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:
[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive
alexfh added a comment. 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)? 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
aaron.ballman added inline comments. Comment at: docs/clang-tidy/index.rst:280 +lint-command +lint-command lint-args + xgsa wrote: > 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? I think that's sufficient, thanks! 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 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. Comment at: docs/clang-tidy/index.rst:256 +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. s/preferable/preferred Comment at: docs/clang-tidy/index.rst:257 +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 s/mute/silence Comment at: docs/clang-tidy/index.rst:261 +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 At the end of this sentence, I would put "However, there are times". e.g., However, there are times when it is more appropriate to silence the diagnostic instead of changing the semantics of the code. In these circumstances, the NOLINT or NOLINTNEXTLINE comments can be used to silence the diagnostic. 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
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
[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive
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
alexfh added inline comments.
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:294
-static bool LineIsMarkedWithNOLINT(SourceManager , SourceLocation Loc) {
+static bool IsNOLINTFound(StringRef NolintMacro, StringRef Line,
+ unsigned DiagID, const ClangTidyContext ) {
"NOLINT" is not a "macro". Maybe `NolintDirective`, `NolintSpelling`,
`NolintWord` or something like this?
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:297
+ const size_t NolintIndex = Line.find(NolintMacro);
+ if (NolintIndex != StringRef::npos) {
+size_t BracketIndex = NolintIndex + NolintMacro.size();
Please use early returns instead of nested `if`s.
http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
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
alexfh 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 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. 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
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
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from a minor commenting nit, this LGTM. Thanks! Comment at: docs/clang-tidy/index.rst:280 +lint-command +lint-command lint-args + 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` ``` 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
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
+++
[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive
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
+++
[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive
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
aaron.ballman added inline comments.
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:299
+size_t BracketIndex = NolintIndex + NolintMacro.size();
+// Check if the specific checks are specified in brackets
+if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
Comments should be complete sentences, including punctuation (here and
elsewhere).
Comment at: docs/ReleaseNotes.rst:259-260
+- Added an ability to specify in parentheses the list of checks to suppress
for the ``NOLINT``
+ and ``NOLINTNEXTLINE`` comments.
+
How about: "Added the ability to suppress specific checks (or all checks) in a
NOLINT or NOLINTNEXTLINE comment."?
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
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).
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);
Is the space before the `(` intended?
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
JonasToth added a comment. I am happy with everything now. But one of the reviewers must accept. 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
JonasToth added inline comments.
Comment at: test/clang-tidy/nolintnextline.cpp:23
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };
xgsa wrote:
> 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
> ```
Ah sure, that makes sense.
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
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
JonasToth added a comment.
Its good that you added code examples to the clang-tidy documentation page. I
think that feature was not documented well before.
Comment at: test/clang-tidy/nolintnextline.cpp:23
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };
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?
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
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 , SourceLocation Loc) {
+static bool IsNOLINTFound(StringRef NolintMacro, StringRef Line,
+ unsigned DiagID, const ClangTidyContext ) {
+ const size_t NolintIndex = Line.find(NolintMacro);
+ if (NolintIndex != StringRef::npos) {
+size_t BracketIndex = NolintIndex +
[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive
lebedev.ri added inline comments. Comment at: docs/ReleaseNotes.rst:259 +- The ``NOLINT`` and ``NOLINTNEXTLINE`` suppression comments were extended to support the list of checks + to disable in parentheses. This reads strange, maybe it can be reworded somehow? 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
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 , SourceLocation Loc) {
+static bool IsNOLINTFound(StringRef NolintMacro, StringRef Line,
+ unsigned DiagID, const ClangTidyContext ) {
+ const size_t NolintIndex = Line.find(NolintMacro);
+ if (NolintIndex != StringRef::npos) {
+
[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive
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 , SourceLocation Loc) {
+static bool
[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive
aaron.ballman added a comment. This feature should probably be mentioned in the release notes. 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
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 , SourceLocation Loc) {
+static bool IsNOLINTFound(StringRef NolintMacro, StringRef Line,
+ unsigned DiagID, const ClangTidyContext ) {
+ 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 , SourceLocation Loc,
+ unsigned DiagID,
+ const ClangTidyContext ) {
bool Invalid;
const char *CharacterData = SM.getCharacterData(Loc, );
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",
[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive
Eugene.Zelenko added inline comments.
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:293
}
-
-static bool LineIsMarkedWithNOLINT(SourceManager , SourceLocation Loc) {
+static bool IsNOLINTFound(StringRef NolintMacro, StringRef Line,
+ unsigned DiagID, const ClangTidyContext ) {
Please separate with empty line.
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:323
+}
+static bool LineIsMarkedWithNOLINT(SourceManager , SourceLocation Loc,
+ unsigned DiagID,
Please separate with empty line.
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
aaron.ballman added inline comments.
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:295
+ unsigned DiagID, const ClangTidyContext ) {
+ const auto NolintIndex = Line.find(NolintMacro);
+ if (NolintIndex != StringRef::npos) {
Only use `auto` when the type is spelled out explicitly in the initialization
(usually through a cast or constructor). Same comment applies elsewhere.
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:311-313
+ for (auto : Checks) {
+Check = Check.trim();
+ }
`llvm::transform(Checks, Checks.begin(), [](StringRef S) { return S.trim(); });`
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:314
+ }
+ return std::find(Checks.begin(), Checks.end(), CheckName) !=
+ Checks.end();
Can use `llvm::find(Checks, CheckName)`
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
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 , SourceLocation Loc) {
+static bool IsNOLINTFound(StringRef NolintMacro, StringRef Line,
+ unsigned DiagID, const ClangTidyContext ) {
+ 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 : Checks) {
+Check = Check.trim();
+ }
+ return std::find(Checks.begin(), Checks.end(), CheckName) !=
+ Checks.end();
+}
+ }
+}
+return true;
+ }
+ return false;
+}
+static bool LineIsMarkedWithNOLINT(SourceManager , SourceLocation Loc,
+ unsigned DiagID,
+ const ClangTidyContext ) {
bool Invalid;
const char *CharacterData = SM.getCharacterData(Loc, );
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") !=
[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive
JonasToth 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); };
xgsa wrote:
> 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?
No i mean an errornous NOLINT.
Like `// NOLINT some_check, othercheck, blaa`. One might expect it to work, but
the missing parens would inhibit that expected behaviour.
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
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 , SourceLocation Loc) {
+static bool IsNOLINTFound(StringRef NolintMacro, StringRef Line,
+ unsigned DiagID, const ClangTidyContext ) {
+ 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 : Checks) {
+Check = Check.trim();
+ }
+ return std::find(Checks.begin(), Checks.end(), CheckName) !=
+ Checks.end();
+}
+ }
+}
+return true;
+ }
+ return false;
+}
+static bool LineIsMarkedWithNOLINT(SourceManager , SourceLocation Loc,
+ unsigned DiagID,
+ const ClangTidyContext ) {
bool Invalid;
const char *CharacterData = SM.getCharacterData(Loc, );
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", RestOfLine, DiagID, Context))
[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive
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
JonasToth 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); };
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?
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
