This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Support globbing in NOLINT* expressions
ClosedPublic

Authored by carlosgalvezp on Oct 6 2021, 12:07 AM.

Details

Summary

To simplify suppressing multiple warnings, e.g. coming from check aliases.

The code for globbing is the same as for enabling checks, so the semantics are identical. Naturally, the meaning is reversed since we are _disabling_ warnings in NOLINT expressions.

This patch also simplifies the logic of NOLINTBEGIN/END, removing the division between "global" vs "specific" suppressions. It's not clear whether // NOLINTBEGIN(google*) is global or specific, for example.

Instead, keep it easy: NOLINTBEGIN(X) must have a matching NOLINTEND(X), for any X. This enforcement leads to easier to read code on the user side.

Diff Detail

Event Timeline

carlosgalvezp created this revision.Oct 6 2021, 12:07 AM
carlosgalvezp requested review of this revision.Oct 6 2021, 12:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2021, 12:07 AM

Fixed formatting.

Update comment in code.

Remove empty lines

Remove redundant "*" if branch.

Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
73

Please separate with newline.

Fixed comments.

carlosgalvezp marked an inline comment as done.Oct 6 2021, 7:51 AM
clang-tools-extra/docs/clang-tidy/index.rst
360

Would anyone do this on purpose, or is this a user error?

carlosgalvezp added inline comments.Oct 7 2021, 11:52 PM
clang-tools-extra/docs/clang-tidy/index.rst
360

I don't see any use case here, no. I just thought to document it to clarify the double negation that's happening, in case a user gets confused and doesn't see the warning being suppressed.

Do you think it brings value or does more harm than good?

What should happen with the following code:

// NOLINTBEGIN(google*)

// NOLINTEND(google-explicit-constructor)
// NOLINTEND(google-runtime-operator)

and this code:

// NOLINTBEGIN(google-explicit-constructor)
// NOLINTBEGIN(google-runtime-operator)

// NOLINTEND(google*)

At the moment, Clang-Tidy does not allow mixing "check-specific" NOLINTBEGIN/ENDs with "global" ones. See discussion in D108560#3020444. So the following is not a supported construct:

// NOLINTBEGIN(google-explicit-constructor)
// NOLINTEND(*)

How should Clang-Tidy behave when mixing check-specific NOLINTBEGIN/ENDs with globbed ones?

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
350

What happens when CheckStr is empty?
How has Clang-Tidy treated // NOLINT() in the past? Does this patch change the behaviour? What should be the "right" behaviour?

354

So when this function is called with args Line = "NOLINTBEGIN(google*)" and CheckName = "google-explicit-constructor", it will return true with *SuppressionIsSpecific = true.
Should *SuppressionIsSpecific = false instead here?

clang-tools-extra/docs/clang-tidy/index.rst
354

To match the spacing in the above lines.

357
360
clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
66

How about changing class D6 so that it violates 2 checks: a google one and a non-google one.
Then you can confirm whether the * in NOLINT(*,-google*) is working.

How should Clang-Tidy behave when mixing check-specific NOLINTBEGIN/ENDs with globbed ones?

I would say keep current behavior - complain that the user wrote something different in the BEGIN and in the END. I like keeping things simple and easy to read and reason about. I think adding smarter logic here would make clang-tidy's code more complicated, but also it would allow users to write more complicated NOLINT expressions that would be very hard to read and track.

Honestly as a user I'm more than fine with one level of NOLINTBEGIN/END. Any more nesting than that causes me much more cognitive effort than it's worth.

What do you think?

carlosgalvezp added inline comments.Oct 8 2021, 4:09 AM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
354

Good point. I honestly don't understand what SuppressionIsSpecific means and how it's used downstream, could you clarify?

How should Clang-Tidy behave when mixing check-specific NOLINTBEGIN/ENDs with globbed ones?

I would say keep current behavior - complain that the user wrote something different in the BEGIN and in the END. I like keeping things simple and easy to read and reason about. I think adding smarter logic here would make clang-tidy's code more complicated, but also it would allow users to write more complicated NOLINT expressions that would be very hard to read and track.

Honestly as a user I'm more than fine with one level of NOLINTBEGIN/END. Any more nesting than that causes me much more cognitive effort than it's worth.

What do you think?

I think the current simple approach is the way to go. If and when someone comes back to us with a compelling case for why more complicated NOLINT syntax is needed, we can think about it then.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
354

SuppressionIsSpecific = true means when a check is suppressed by name, i.e. NOLINTBEGIN(check-name).

SuppressionIsSpecific = false means when NOLINTBEGIN <zero args> or NOLINTBEGIN(*) is used. My gut feeling is that It should probably be false when a glob is used too.

The point of this variable is so that a check-specific BEGIN can only be ENDed with a check-specific END.

clang-tools-extra/docs/clang-tidy/index.rst
360

I don't see any use case here, no. I just thought to document it to clarify the double negation that's happening, in case a user gets confused and doesn't see the warning being suppressed.

Do you think it brings value or does more harm than good?

I'd be happy with just the basic + glob functionality. The first thing I'd use it on is to suppress checks that are an alias of another check, e.g. NOLINT(*no-malloc) to cover both hicpp-no-malloc and cppcoreguidelines-no-malloc.

As for glob expressions that begin with a -... you get the functionality for free thanks to the GlobList class but it's not a feature I think I will need. I speak only for myself though. Maybe someone else here has a strong need for this?

Is NOLINTBEGIN(-check) equivalent to NOLINTEND(check)? It hursts my head even thinking about it.

Your unit test where you test NOLINT(google*,-google*,google*), Clang-Tidy does the right thing in that situation, but I have to wonder why any user would want to add, remove, then add the same check group in the one expression in the first place? Should we even be entertaining this kind of use?

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
354

I think the behaviour has changed in this patch.

It used to return *SuppressionIsSpecific = false when ChecksStr = "*".
"*" is disabling all checks, not a specific check.

Now it returns *SuppressionIsSpecific = true. Globs.contains(CheckName) is true regardless of whether ChecksStr = "*" or ChecksStr = "check-name", so it continues running onto line 353 and sets *SuppressionIsSpecific = true no matter what.

carlosgalvezp added inline comments.Oct 9 2021, 1:34 AM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
354

You are right! It's interesting however that this hasn't triggered any unit test error, so I wonder if this variable is needed at all then? I will have a deeper look at the code to get a better understanding and update the patch accordingly.

clang-tools-extra/docs/clang-tidy/index.rst
360

Is NOLINTBEGIN(-check) equivalent to NOLINTEND(check)?

To me it's clear that it's not. Any NOLINTBEGIN(X) must have a matching NOLINTEND(X), regardless of whether X is a glob or not.

Totally agree that negative globs are probably not needed, the main use case is being able to suppress all aliases as you say.

I just thought it was neat to be able to reuse code in that way, the code is very clean and easy to read. Plus users are familiar with the syntax. If users want to abuse it that would be at their own maintenance expense - clang-tidy would just do what it was asked to do, without crashing or anything bad happening.

The same thing can be done when running the tool - you can run "--checks=google*,-google*" and I don't think clang-tidy has code to warn the user/prevent this from happening. Considering all these edge cases would perhaps complicate the design of the tool for no real use case? I.e. too defensive programming.

I added the unit test mostly to make sure clang-tidy doesn't crash or does something strange, it just does what the user instructed it to do. I thought such edge cases are encouraged in unit tests to increase coverage.

Would it be reasonable to keep negative globs in the implementation (to simplify maintenance, reuse code), but not document it in the public docs? Otherwise I'll need to refactor GlobList to be able to reuse the part of the code that consumes the positive globs. I don't think it makes sense to create a parallel implementation of that part (kind of what exists already now, before my patch).

Looking at the code, I see that you use SuppressionIsSpecific in order to separate the errors into 2 error lists: SpecificNolintBegins and GlobalNolintBegins. However you don't do anything different with either list, so why do they need to be different lists?

Here checking that a combined list is empty would be equivalent:

bool WithinNolintBegin =
    !SpecificNolintBegins.empty() || !GlobalNolintBegins.empty();

And here, you are running identical code for both lists:

for (const auto NolintBegin : SpecificNolintBegins) {
  auto Error = createNolintError(Context, SM, NolintBegin, true);
  SuppressionErrors.emplace_back(Error);
}
for (const auto NolintBegin : GlobalNolintBegins) {
  auto Error = createNolintError(Context, SM, NolintBegin, true);
  SuppressionErrors.emplace_back(Error);
}

And then these lists are not used any further than the scope of the function where they are declared. So to me it feels like they could be combined, and this logic of SuppressionIsSpecific be removed. Let me know if I'm missing something obvious!

carlosgalvezp marked an inline comment as done.Oct 9 2021, 1:54 AM
carlosgalvezp added inline comments.
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
350

Very good question! Currently on master // NOLINT() will *not* suppress warnings. However // NOLINT( will. My patch shouldn't change existing behavior - an empty list of globs will return false when calling contains.

I'll add a unit test to cover this case!

carlosgalvezp marked 5 inline comments as done.

-Fixed formatting comments.
-Added test for NOLINT(), NOLINTNEXTLINE() and NOLINTBEGIN()
-Changed last test to more effectively test NOLINT(*,-google*)

Looking at the code, I see that you use SuppressionIsSpecific in order to separate the errors into 2 error lists: SpecificNolintBegins and GlobalNolintBegins. However you don't do anything different with either list, so why do they need to be different lists?

Here checking that a combined list is empty would be equivalent:

bool WithinNolintBegin =
    !SpecificNolintBegins.empty() || !GlobalNolintBegins.empty();

And here, you are running identical code for both lists:

for (const auto NolintBegin : SpecificNolintBegins) {
  auto Error = createNolintError(Context, SM, NolintBegin, true);
  SuppressionErrors.emplace_back(Error);
}
for (const auto NolintBegin : GlobalNolintBegins) {
  auto Error = createNolintError(Context, SM, NolintBegin, true);
  SuppressionErrors.emplace_back(Error);
}

And then these lists are not used any further than the scope of the function where they are declared. So to me it feels like they could be combined, and this logic of SuppressionIsSpecific be removed. Let me know if I'm missing something obvious!

tallyNolintBegins() goes through every line, and whenever it sees a NOLINTBEGIN, it pushes its SourceLocation onto a stack. When it sees a NOLINTEND, it pops one entry off the stack. At the end of the file, all entries on the stack should have been popped off - if not, it's a unmatched NOLINTBEGIN/END expression error.

NOLINTBEGIN(check-name) expressions are pushed onto the SpecificNolintBegins list, while NOLINTBEGIN & NOLINTBEGIN(*) expressions are pushed onto the GlobalNolintBegins list. The reason for the two lists is so that NOLINTBEGIN(check-name) cannot be popped off by NOLINTEND or NOLINTEND(*); and NOLINTBEGIN and NOLINTBEGIN(*) cannot be popped off by a NOLINTEND(check-name).

See this line in tallyNolintBegins() where it figures out which list to use for the NOLINTBEGIN/END expression currently being evaluated...

auto List = [&]() -> SmallVector<SourceLocation> * {
  return SuppressionIsSpecific ? &SpecificNolintBegins : &GlobalNolintBegins;
};

I'm sure there is an alternate implementation which achieves the same thing with just one list object... however the list will probably need to store more information about the NOLINTBEGIN expression than just its SourceLocation to be able to achieve the same goals though... probably a list of pair<SourceLocation Location, StringRef CheckName>, otherwise you can't match a NOLINTEND to its corresponding NOLINTBEGIN. If you want to take a crack at simplifying the logic here, all improvements are appreciated.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
354

You are right! It's interesting however that this hasn't triggered any unit test error

That's because the unit tests are missing a test case that checks this specific combination of BEGIN/END expressions. The tests predominantly use NOLINTBEGIN <zero args>, not NOLINTBEGIN(*).
It would be great if this shortcoming in the test coverage could be plugged in this patch.

nolintbeginend-begin-global-end-specific.cpp checks:

// NOLINTBEGIN
class A { A(int i); };
// NOLINTEND(google-explicit-constructor)

nolintbeginend-begin-specific-end-global.cpp checks:

// NOLINTBEGIN(google-explicit-constructor)
class A { A(int i); };
// NOLINTEND

These 2 files could be used as the basis to create tests for NOLINTBEGIN(*) / NOLINTEND(*).

clang-tools-extra/docs/clang-tidy/index.rst
360

Is NOLINTBEGIN(-check) equivalent to NOLINTEND(check)?

To me it's clear that it's not. Any NOLINTBEGIN(X) must have a matching NOLINTEND(X), regardless of whether X is a glob or not.

Totally agree that negative globs are probably not needed, the main use case is being able to suppress all aliases as you say.

I just thought it was neat to be able to reuse code in that way, the code is very clean and easy to read. Plus users are familiar with the syntax. If users want to abuse it that would be at their own maintenance expense - clang-tidy would just do what it was asked to do, without crashing or anything bad happening.

The same thing can be done when running the tool - you can run "--checks=google*,-google*" and I don't think clang-tidy has code to warn the user/prevent this from happening. Considering all these edge cases would perhaps complicate the design of the tool for no real use case? I.e. too defensive programming.

I added the unit test mostly to make sure clang-tidy doesn't crash or does something strange, it just does what the user instructed it to do. I thought such edge cases are encouraged in unit tests to increase coverage.

I see all your points about reusing the glob functionality. As long as this functionality does not let a user shoot themselves in the foot, I don't see a problem with it.
A large proportion of the review of my NOLINTBEGIN/NOLINTEND patch was spent going over all the ways a user could misuse the functionality. The concern was mainly that we didn't want a user to unwittingly disable checks for the entire file because of a stray NOLINTBEGIN without an NOLINTEND marker.
The worst I can see a confused user do with this new glob feature is use NOLINT(-check*) which as you have already explained is a double negative and shows the warnings anyway, so no harm done.

I find it funny that master allows NOLINT( with no closing bracket go though as a NOLINT when it's clearly a sign of user error. Clang-Tidy could be more proactive in alerting the user to these mistakes, but I get there's a balance between code complexity and harm reduction. This quirk is not directly related to your patch so that's a story to continue on another day.

Would it be reasonable to keep negative globs in the implementation (to simplify maintenance, reuse code), but not document it in the public docs? Otherwise I'll need to refactor GlobList to be able to reuse the part of the code that consumes the positive globs. I don't think it makes sense to create a parallel implementation of that part (kind of what exists already now, before my patch).

I'm only here to provide some insight about code I recently wrote. Let's wait for the actual reviewers who have a better sense of the direction of this project to say how things should be.

Thanks for the clarification, makes a lot of sense now! I'll see what I can do with that.

By the way, is NOLINTBEGIN/END expected to work/give errors when the check name is something else than a real check name? See for example:

https://godbolt.org/z/b6cbTeezs

I would expect to get a warning that an END was found that didn't match a BEGIN.

By the way, is NOLINTBEGIN/END expected to work/give errors when the check name is something else than a real check name? See for example:

https://godbolt.org/z/b6cbTeezs

I would expect to get a warning that an END was found that didn't match a BEGIN.

The functions that we have been discussing in this patch only run when Clang-Tidy has detected a check violation and is about to print a warning about it. The NOLINT comment checks happen at the last minute. Since foo and bar are not real checks, Clang-Tidy won't find any violations, and so the NOLINT checks won't get the opportunity to run.

Refactored NOLINTBEGIN/END slightly. Now, the requirements for a match are stricter (and simpler), making the code easier to reason about: any NOLINTBEGIN(X) must be matched by an identical NOLINTEND(X), for any X.

Added additional tests.

@salman-javed-nz let me know what you think!

carlosgalvezp edited the summary of this revision. (Show Details)Oct 10 2021, 2:58 AM
carlosgalvezp added inline comments.Oct 10 2021, 3:10 AM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
446

I had to remove these "return false", otherwise I would not get errors regarding "Found NOLINTBEGIN that was unmatched by a NOLINTEND". Not really sure why they were there in the first place.

All unit tests pass with this patch.

Now, the requirements for a match are stricter (and simpler), making the code easier to reason about: any NOLINTBEGIN(X) must be matched by an identical NOLINTEND(X), for any X.

Thanks! Simplifying it right down looks to me like the right thing to do.

In case you find it hard to follow my suggested changes in Phabricator's interface, I have copied these suggestions into this attachment for convenience of reading:

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
325

This feels like a more intuitive argument name to me, plus it aligns with the local variable ChecksStr below. What do you think?

343–356

I think this should work the same without the additional index variable and substring.

344

This is just a copy of BracketIndex. Would it be better to consolidate the two?

347–352

It gets a bit hard to follow with BracketIndex and BracketStartIndex interspersed on these lines, and a + 1 in one substring but not in the other substring.
Also aren't both substrings pretty much the same thing?

395

To align with the name ChecksStr used in the other function(s).

405–408

It's not necessarily the last element of NolintBegins that you need to pop_back().

In the case where you have overlapping NOLINTBEGIN regions...

// NOLINTBEGIN(A)  <-- push A
// NOLINTBEGIN(B) <-- push B
// NOLINTEND(A) <-- pop A
// NOLINTEND(B) <-- pop B

Therefore you need to search through all of the elements, not just the last one.

clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
57

Since C6 & C7 have been removed you could renumber C8 to C6. Once this feature is in master I don't see this test file changing again for a long time.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
446

The reason for the return false was because the file is already determined to have a unmatched NOLINTBEGIN, so it's FUBAR and probably not worth checking the remainder of anyway. I don't mind either way.

carlosgalvezp marked 9 inline comments as done.

Fixed comments from @salman-javed-nz

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
325

Sure. What about FoundNolintChecksStr, for consistency with FoundNolintIndex?

347–352

I wanted "FoundCheckStr" to contain also the brackets, so that NOLINT( is different than NOLINT(). However I realize now that they are anyway treated differently (as per previous discussion) so I'll remove this and keep it simple.

405–408

Thanks, that's right. However that's a problem that already exists on master, right? It's pushing from the back anyway? We should probably have a unit test for this.

Would it make sense to leave that for a separate patch? Seems like the scope is growing and I'd like to keep the change as small as possible.

446

I see. In the test case that I added, I got prints that NOLINTEND is unmatched by a BEGIN, but I didn't get a message that NOLINTBEGIN is unmatched by an END, which I did get in a separate unit test, so I thought that's intended behavior.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
405–408

Thanks, that's right. However that's a problem that already exists on master, right? It's pushing from the back anyway?

Actually yes, you're right. Disregard my previous comment. I was thinking of a scenario that isn't a problem.
It's all looking good to me now.

salman-javed-nz added a comment.EditedOct 10 2021, 1:51 PM

Thanks for all the changes. You've addressed all my questions and I don't have anything more to add. Let's wait for the reviewers to take a look.

Awesome, thanks a lot for the review, I really improved my understanding of the tool :)

@aaron.ballman Could you review? Otherwise could you point me to who should review? I tagged @alexfh as owner of clang-tidy but haven't really seen him around... Shouldn't there be multiple owners, so that there's not a "single point of failure"?

Awesome, thanks a lot for the review, I really improved my understanding of the tool :)

@aaron.ballman Could you review? Otherwise could you point me to who should review? I tagged @alexfh as owner of clang-tidy but haven't really seen him around... Shouldn't there be multiple owners, so that there's not a "single point of failure"?

I'm happy to review (I'll add a few more folks to the list though). Just an FYI, but we usually only ping when there's been about a week of no traffic on the review. Reviewers have a fairly heavy workload and we've settled on a week between pings as being the happy medium.

Thanks for the clarification, makes a lot of sense now! I'll see what I can do with that.

By the way, is NOLINTBEGIN/END expected to work/give errors when the check name is something else than a real check name? See for example:

https://godbolt.org/z/b6cbTeezs

I would expect to get a warning that an END was found that didn't match a BEGIN.

If the tags don't match (so there's an open for a check but never a close for the check), then we should diagnose. However, we should *not* diagnose unknown check names (because the user may be suppressing checks that clang-tidy doesn't know about, such as ones in another static analysis tool run over the same code). However, given that we don't run the NOLINT pass unless there are diagnostics to emit, I think it's fine to not diagnose a mismatch between unknown tags.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
350

(All of this is a bit orthogonal to this patch, so no changes requested here.)

Naively, I would expect NOLINT() to be an error because it expects an argument and none was given. (I'd expect NOLINT( to also be an error because the syntax is malformed.)

clang-tools-extra/docs/clang-tidy/index.rst
360

I see all your points about reusing the glob functionality. As long as this functionality does not let a user shoot themselves in the foot, I don't see a problem with it.

Likewise; code reuse is a good thing when it works in our favor.

The worst I can see a confused user do with this new glob feature is use NOLINT(-check*) which as you have already explained is a double negative and shows the warnings anyway, so no harm done.

I have to think on this more, but my initial reaction is that it's a bit scary because the check names themselves have dashes within them already, but those dashes don't mean negation. So I worry about this being confusing:

// NOLINT(misc-whatever-awesome)
// Oops, we didn't notice that there's also bugprone-whatever-awesome, 
// so let's try to suppress them all.

// NOLINT(-whatever-awesome)
// Oops. that actual disables the check because we didn't know to add the star. But because it's in a NOLINT, that's a double negation, so we've reenabled the check. So the user will still get diagnostics, but likely not understand why.

// NOLINT(*-whatever-awesome)
// This was what we meant to write.

I don't think allowing users to do double-negations in NOLINT comments adds a lot of value. Would it be reasonable to pass in a flag to GlobList to disable negative glob handling so we can still reuse the logic but not have to enable a weird feature that users may accidentally run into?

carlosgalvezp marked an inline comment as done.

Remove support for negative globs. Keep unit tests to ensure that negative globs are ignored. Update documentation.

carlosgalvezp marked an inline comment as done.Oct 12 2021, 1:16 AM
carlosgalvezp added inline comments.
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
350

Fully agree! If I find some time I can give it a try on a separate patch

Just an FYI, but we usually only ping when there's been about a week of no traffic on the review

Thanks, I'll keep it in mind for next time! Really appreciate your time reviewing :)

I have updated the patch based on your feedback.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
350

The NOLINT syntax was borrowed from Google's cpplint.py. cpplint uses a regex search to find NOLINT expressions, but it's pretty naive, leading to quirks like NOLINT( being accepted as a valid expression. Clang-Tidy has inherited the same quirks by being interoperable with cpplint's syntax.

How important it is to maintain compatibility with the more funky aspects of cpplint's NOLINT behaviour, I'm not sure. The two programs' syntaxes aren't fully compatible nowadays anyway. To give an example, cpplint doesn't support multiple checks separated by commas on the same line. This glob feature will be another divergence.

Just something to think about for next time.

clang-tools-extra/docs/clang-tidy/index.rst
314

There's something about this "causing confusion to the users" that my mind latches on to. I'm not sure what my problem with it is. Perhaps the sentence will read better if you just remove the 2nd half?

carlosgalvezp marked an inline comment as done.

Addressed @salman-javed-nz comment.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
350

I'd gladly decouple clang-tidy and cpplint. Personally I don't find the benefit of running both, clang-tidy is way superior.

Expand documentation to make it clear that globbing is supported for NOLINT, NOLINTNEXTLINE as well as NOLINTBEGIN/END.

aaron.ballman added inline comments.Oct 19 2021, 6:20 AM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
350

How important it is to maintain compatibility with the more funky aspects of cpplint's NOLINT behaviour, I'm not sure.

I think the important thing is for users who use both tools to not have a bad experience because clang-tidy is yelling at them about NOLINT comment syntax. Fixing a syntax issue to make clang-tidy happy only to break cpplint (and vice versa) would leave users stuck.

I don't have a copy of cpplint handy, so I'm not able to validate my own suggestions for diagnostics here. If you hear a suggestion that would lead us down the "warring diagnostics" path, please push back!

352
406
454–455
carlosgalvezp marked 6 inline comments as done.Oct 19 2021, 6:50 AM
carlosgalvezp marked 3 inline comments as done.

Addressed comments.

aaron.ballman accepted this revision.Oct 19 2021, 7:46 AM

LGTM, but if someone can verify we aren't introducing dueling diagnostics with cpplint before we land this, that would be appreciated.

This revision is now accepted and ready to land.Oct 19 2021, 7:46 AM

Actually, cpplint is never compatible with clang-tidy regardless. The following code:

struct Foo
{
	Foo(int) {} // NOLINT(google-explicit-constructor)
};

Triggers:

../test.cpp:3:  Unknown NOLINT error category: google-explicit-constructor  [readability/nolint] [5]

So one has to disable the diagnostic anyway via --filter=-readability/nolint. I've tested using globs and cpplint works fine so long the diagnostic is suppressed. I.e. this patch doesn't change things in that respect.

I go ahead and merge if you are happy then!

A bit annoying about the build bot, fixed in a separate commit. Do you know if we can run these jobs pre-pushing? Seems rather unlikely to have them all pass the first time, especially for larger commits...

Actually, cpplint is never compatible with clang-tidy regardless. The following code:

struct Foo
{
	Foo(int) {} // NOLINT(google-explicit-constructor)
};

Triggers:

../test.cpp:3:  Unknown NOLINT error category: google-explicit-constructor  [readability/nolint] [5]

So one has to disable the diagnostic anyway via --filter=-readability/nolint. I've tested using globs and cpplint works fine so long the diagnostic is suppressed. I.e. this patch doesn't change things in that respect.

I go ahead and merge if you are happy then!

Oh, that's a bummer. :-( Oh well, your patch doesn't make that any worse.

A bit annoying about the build bot, fixed in a separate commit. Do you know if we can run these jobs pre-pushing? Seems rather unlikely to have them all pass the first time, especially for larger commits...

Unfortunately, precommit CI is pretty limited and extremely flakey, so the only real way to know right now is to push and see how the bot farm likes it. (There are a lot of bots we don't want to use for precommit CI because they take a long time to run, use experimental or flakey hardware, etc.)

Unfortunately, precommit CI is pretty limited and extremely flakey, so the only real way to know right now is to push and see how the bot farm likes it. (There are a lot of bots we don't want to use for precommit CI because they take a long time to run, use experimental or flakey hardware, etc.)

I see, I'll make sure to plan for watching the build bots post-merge then. Thanks!