Page MenuHomePhabricator

[clang-tidy] Support specific checks for NOLINT directive
ClosedPublic

Authored by xgsa on Nov 30 2017, 1:25 PM.

Details

Summary

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)"

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
JonasToth added inline comments.Nov 30 2017, 11:20 PM
clang-tidy/ClangTidyDiagnosticConsumer.cpp
298

Maybe some comments whats happening might be helpfull. It would simplify later modifications.

test/clang-tidy/nolintnextline.cpp
14

missing )

xgsa added a comment.Dec 1 2017, 12:40 AM

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

xgsa retitled this revision from [clang-tidy] Support specific categories for NOLINT directive to [clang-tidy] Support specific checks for NOLINT directive.Dec 1 2017, 12:41 AM
xgsa edited the summary of this revision. (Show Details)
JonasToth added inline comments.Dec 1 2017, 1:26 AM
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?

xgsa added inline comments.Dec 1 2017, 1:37 AM
test/clang-tidy/nolintnextline.cpp
14

There is such test case (for class "B") or did you mean something else?

xgsa updated this revision to Diff 125084.Dec 1 2017, 1:52 AM

Add comments to code as it was recommended.

xgsa marked an inline comment as done.Dec 1 2017, 1:52 AM
JonasToth added inline comments.Dec 1 2017, 2:11 AM
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.

xgsa updated this revision to Diff 125096.Dec 1 2017, 3:34 AM

A few additional test cases were added.

xgsa marked an inline comment as done.Dec 1 2017, 3:34 AM
aaron.ballman added inline comments.Dec 1 2017, 6:41 AM
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)

Eugene.Zelenko added inline comments.
clang-tidy/ClangTidyDiagnosticConsumer.cpp
294–325

Please separate with empty line.

324

Please separate with empty line.

Eugene.Zelenko edited reviewers, added: hokein; removed: Restricted Project.Dec 1 2017, 6:59 AM
Eugene.Zelenko set the repository for this revision to rL LLVM.
xgsa updated this revision to Diff 125140.Dec 1 2017, 7:21 AM
xgsa marked 5 inline comments as done.
aaron.ballman edited edge metadata.Dec 1 2017, 9:27 AM

This feature should probably be mentioned in the release notes.

xgsa updated this revision to Diff 125190.Dec 1 2017, 12:14 PM

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.

xgsa updated this revision to Diff 125216.Dec 1 2017, 1:38 PM

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

lebedev.ri added inline comments.Dec 2 2017, 12:09 AM
docs/ReleaseNotes.rst
259

This reads strange, maybe it can be reworded somehow?

xgsa updated this revision to Diff 125258.Dec 2 2017, 1:01 AM

Release note item was reworded

xgsa marked an inline comment as done.Dec 2 2017, 1:02 AM

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?

xgsa added inline comments.Dec 2 2017, 6:37 AM
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
JonasToth added inline comments.Dec 2 2017, 7:44 AM
test/clang-tidy/nolintnextline.cpp
23

Ah sure, that makes sense.

I am happy with everything now. But one of the reviewers must accept.

aaron.ballman added inline comments.Dec 3 2017, 7:51 AM
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?

xgsa marked 2 inline comments as done.Dec 3 2017, 9:02 AM
xgsa added inline comments.
docs/clang-tidy/index.rst
270

No, it's a mistake, thank you.

xgsa updated this revision to Diff 125291.Dec 3 2017, 9:03 AM

Updated documentation and comments in code.

xgsa marked 2 inline comments as done.Dec 3 2017, 9:04 AM
xgsa updated this revision to Diff 125292.Dec 3 2017, 9:07 AM

A typo in documentation was fixed.

aaron.ballman accepted this revision.Dec 3 2017, 11:12 AM

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`
This revision is now accepted and ready to land.Dec 3 2017, 11:12 AM
xgsa added inline comments.Dec 3 2017, 12:42 PM
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).
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?

alexfh added inline comments.Dec 4 2017, 2:38 AM
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.

alexfh added inline comments.Dec 4 2017, 2:45 AM
clang-tidy/ClangTidyDiagnosticConsumer.cpp
294

"NOLINT" is not a "macro". Maybe NolintDirective, NolintSpelling, NolintWord or something like this?

297
xgsa marked 2 inline comments as done.Dec 4 2017, 6:18 AM
xgsa added inline comments.
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.

xgsa updated this revision to Diff 125328.Dec 4 2017, 6:24 AM
xgsa marked an inline comment as done.Dec 4 2017, 6:26 AM
aaron.ballman added inline comments.Dec 4 2017, 10:12 AM
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!

alexfh edited edge metadata.Dec 5 2017, 4:31 AM

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)?

xgsa marked 2 inline comments as done.Dec 5 2017, 6:42 AM
xgsa added inline comments.
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?

xgsa updated this revision to Diff 125517.Dec 5 2017, 6:44 AM

Updated documentation

xgsa added a comment.Dec 5 2017, 8:36 AM

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.

xgsa added a comment.Dec 6 2017, 11:25 PM

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

aaron.ballman requested changes to this revision.Dec 7 2017, 2:08 PM
In 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.

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.

This revision now requires changes to proceed.Dec 7 2017, 2:08 PM
xgsa marked an inline comment as done.Dec 7 2017, 3:01 PM

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!

xgsa updated this revision to Diff 126058.Dec 7 2017, 3:04 PM
xgsa edited edge metadata.

Documentation update

alexfh added a comment.Dec 8 2017, 9:19 AM
In D40671#945158, @xgsa 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?

xgsa added a comment.Dec 8 2017, 10:03 AM

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.

xgsa added a comment.Dec 12 2017, 11:37 PM

@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?

In 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.

aaron.ballman accepted this revision.Dec 13 2017, 7:27 AM
This revision is now accepted and ready to land.Dec 13 2017, 7:27 AM
alexfh accepted this revision.Dec 13 2017, 4:22 PM
In D40671#949732, @xgsa 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 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.

xgsa added a comment.Dec 13 2017, 11:56 PM

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?

In D40671#954906, @xgsa 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.

aaron.ballman closed this revision.Dec 14 2017, 8:25 AM

Committed in r320713.

In D40671#954906, @xgsa 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.