The NOLINT directive was extended to support the "NOLINT(check)" and "NOLINT(*)" syntax. Also it is possible to specify a few checks separated with comma "NOLINT(check1, check2)"
Details
Diff Detail
Event Timeline
Please upload patches with full context (-U999999)
Also, you probably want to add some reviewers, see CODE_OWNERS.txt e.g.
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.
test/clang-tidy/nolintnextline.cpp | ||
---|---|---|
14 | No, it's intentionally: it's a test for case when ) is missing |
test/clang-tidy/nolintnextline.cpp | ||
---|---|---|
14 | 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? |
test/clang-tidy/nolintnextline.cpp | ||
---|---|---|
14 | There is such test case (for class "B") or did you mean something else? |
test/clang-tidy/nolintnextline.cpp | ||
---|---|---|
14 | 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. |
clang-tidy/ClangTidyDiagnosticConsumer.cpp | ||
---|---|---|
296 | Only use auto when the type is spelled out explicitly in the initialization (usually through a cast or constructor). Same comment applies elsewhere. | |
312โ314 | llvm::transform(Checks, Checks.begin(), [](StringRef S) { return S.trim(); }); | |
315 | Can use llvm::find(Checks, CheckName) |
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.
docs/ReleaseNotes.rst | ||
---|---|---|
259 | This reads strange, maybe it can be reworded somehow? |
Its good that you added code examples to the clang-tidy documentation page. I think that feature was not documented well before.
test/clang-tidy/nolintnextline.cpp | ||
---|---|---|
23 | 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? |
test/clang-tidy/nolintnextline.cpp | ||
---|---|---|
23 | 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 |
test/clang-tidy/nolintnextline.cpp | ||
---|---|---|
23 | Ah sure, that makes sense. |
clang-tidy/ClangTidyDiagnosticConsumer.cpp | ||
---|---|---|
299 | Comments should be complete sentences, including punctuation (here and elsewhere). | |
docs/ReleaseNotes.rst | ||
259โ260 | How about: "Added the ability to suppress specific checks (or all checks) in a NOLINT or NOLINTNEXTLINE comment."? | |
docs/clang-tidy/index.rst | ||
253 | 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). | |
270 | Is the space before the ( intended? |
docs/clang-tidy/index.rst | ||
---|---|---|
270 | No, it's a mistake, thank you. |
Aside from a minor commenting nit, this LGTM. Thanks!
docs/clang-tidy/index.rst | ||
---|---|---|
280 | 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` |
docs/clang-tidy/index.rst | ||
---|---|---|
280 | 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). lint-command lint-command lint-args Isn't it enough or are there any suggestions how to handle this? |
docs/clang-tidy/index.rst | ||
---|---|---|
253 | 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. |
clang-tidy/ClangTidyDiagnosticConsumer.cpp | ||
---|---|---|
294 | "NOLINT" is not a "macro". Maybe NolintDirective, NolintSpelling, NolintWord or something like this? | |
297 | Please use early returns instead of nested ifs. http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code |
docs/clang-tidy/index.rst | ||
---|---|---|
253 | Thank you for the examples of the check-specific mute ways, I have looked for that, but haven't found the good ones. |
docs/clang-tidy/index.rst | ||
---|---|---|
254โ255 | 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. | |
256 | s/preferable/preferred | |
257 | s/mute/silence | |
261 | 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. | |
280 | I think that's sufficient, thanks! |
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)?
docs/clang-tidy/index.rst | ||
---|---|---|
254โ255 | 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? |
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.
docs/clang-tidy/index.rst | ||
---|---|---|
254โ255 | 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. |
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.
@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.
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.
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.
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.
"NOLINT" is not a "macro". Maybe NolintDirective, NolintSpelling, NolintWord or something like this?