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

[Aaron Ballman via Phabricator via cfe-commits]

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