Added release note.

In D126138#3529820, @paulaltin wrote:

Thanks for preparing this revision @salman-javed-nz!

Do you think it could be worth adding a few more test cases to cover this? It turned out that this issue wasn't actually specific to multi-line macros (see this comment), so if the test case on line 96 was passing then it must not be fully/correctly testing NOLINT for single-line macros. I guess the only way to do this would be to add more checks to the nolint.cpp file, but I realise that's not a trivial change.

Add another test, to test the specific code sample provided in the GitHub issue (check=cppcoreguidelines-pro-type-member-init).

The pre-merge checks are failing on the clang-format check. Not sure why it's complaining about the formatting of the lit test files - those files have never complied with the project style and have not been checked for style for as long as I remember. Has this policy changed recently?

Renamed test case to better explain what it is that it's testing.

Hi @curdeius, have you had the chance to look at this since the last round of comments? The only thing I think this module still needs for now (until a new generic header guard check is developed), is a check that there aren't consecutive underscores in the output.

LGTM

Just a drive-by comment to say, thank you for taking the time to make this fix. It's a bug I've triggered many times. Great to see it being resolved.

My bad. Test should have called run-clang-tidy with -checks=-some-check in the first test case, and with -config "Checks: -some-check" in the second. In both cases, the disabled check should not appear in the Enabled checks: printout.

In D118104#3292862, @JesApp wrote:

Well, since this was more of source of confusion than actual incorrect behaviour, I don't think there should be a test for it.

In general though, I think the script is complex enough to warrant some testing. That being said: I don't think they should be part of this patch. Also, I'm doing this on company time and I don't think my boss would be to happy if I wrote a testsuite from scratch when I just wanted to fix one bug. :D

Do you reckon it's worth expanding the test: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy.cpp to look at this issue?
There isn't much testing of the run-clang-tidy.py at the moment, but it doesn't have to always remain that way.

In D116085#3272200, @nemanjai wrote:

This is causing buildbot failures with -Werror (i.e. https://lab.llvm.org/buildbot/#/builders/57/builds/14322/steps/5/logs/stdio). Please fix or revert.

Review comment:
formNoLintBlocks() - drop the & so the vector must be std::moved in

ClangTidyContext::shouldSuppressDiagnostic():

• Hook up AllowIO and EnableNoLintBlocks to NoLintDirectiveHandler::shouldSuppress()

LGTM besides a couple of nits. I think the diagnostic message should just say what the problem is, not what the fix is - that's what the FixitHint is for.

Every review comment so far should be addressed now, with the exception of the 2 points below.

Pass the NoLintErrors SmallVector all the way through the call stack, instead of storing it in the class

Replace NoLintTokenizer class with a single function, getNoLints()

Instead of defining a new error type for the NoLintDirectiveHandler, use the pre-existing error type clang::tooling::Diagnostic

NoLintToken:

• remove public getter methods for Type and Pos - just use the fields directly
• consolidate the constructors into one constructor
• add comment about the difference about between Checks = None and Checks = ""
• add back the comment about why negative globs are ignored
• don't lazy compute the glob

Fix errors in comments

Put implementation-related classes in an anonymous namespace

Inline short methods into classes

Store the NoLintDirectiveHandler object directly in ClangTidyContext, instead of doing it through a unique_ptr.

Move AllowIO and EnableNoLintBlocks from ClangTidyContext() back to shouldSuppressDiagnostic().

Thank you very much to both of you for having a look at the patch. Yes, I agree it is a large patch, and I could have done a better job splitting it up into more manageable chunks.

Update according to review (rename NoLintPragmaHandler class to NoLintDirectiveHandler)

In D116085#3257210, @carlosgalvezp wrote:

Amazing job @salman-javed-nz ! Here's some initial comments. Reviewing the NoLintPragmaHandler.cpp will take some more time from me. It would have been easier to see the diff if the contents had been moved as an NFC patch to a separate file, and then applying the optimizations in this patch. But it's done now so I'll deal with it :)

Friendly ping.

Move comments

In D116378#3226780, @carlosgalvezp wrote:

By the way, the similar problem exists in Clang compiler. I have written in cfe-dev, Discourse and submitted an issue without any feedback so far (I understand it's been vacation). Do you think it would be beneficial to implement a similar fix there? I don't know if the fix is as straightforward as in clang-tidy or if it will have to be done on a per-check basis.

Updated according to review comments

• Move NoLintToken and other support classes to the CPP file.
• Create a NoLintPragmaHandler class which owns the cache. Access this class through a PIMPL.
• Use StringSwitch instead of StringMap.
• Store the GlobList object instead of repeatedly constructing temporaries.
• Remove Unknown from the NoLintType enum.
• Add constructors to NoLintToken and NoLintBlockToken that can ensure that the objects are initialized with valid values.
• Move shouldSuppressDiagnostic() into ClangTidyContext. This function took ClangTidyContext as a mutable ref anyway.

This resolves the long-standing gripe I have with clang-tidy raising warnings about GoogleTest macros expanded in my code. Good work.
Do you think we can close issues #44873, #43325, and #31587 now that we have this fix?

Thanks for taking a look. I will update the diff to address your comments. Have a good new years break.

Updated the review's edit permissions. Sorry about that, @kadircet.

Remove unnecessary llvm:: qualification.

In D108560#3192081, @kadircet wrote:

Hi @salman-javed-nz !
Also I am not sure what kind of timelines you have in mind for the performance improvements, but I think it would be great to have them before LLVM-14 release, as otherwise we might have some (extra) discrepancies between clang-tidy CLI vs clangd integration, which would be unfortunate.

Hi @kadircet. Today I fixed the last of the bugs in my caching implementation. I still need to run some tests to quantify the performance improvement before submitting the code for a review.

In D115715#3192085, @aaron.ballman wrote:

It could be made to be useful outside of LLVM, but as it stands today, the check is only intended to be useful for LLVM. If we want to make it useful outside of LLVM, that's fine, but there's a bit more to do (for example, the check should be exposed outside of the llvm module) and that includes generalizing the check.

The problem at the heart of all this is that llvm-header-guard isn't written flexible enough to support non-LLVM project structures.

Hi, just giving a progress update. This is just a early heads-up - no action needed from you at this stage.

The ~6700 instances of // end namespace - do you observe any trend, e.g. are they concentrated in one of the LLVM sub-projects? Is it similar to the situation we have with the readability-identifier-naming check, where a number of sub-projects have disabled it in their sub-directory's .clang-tidy file?

LGTM. The use of mutable with public inheritence is all over the clang-tools-extra code. See class SwapIndex : public SymbolIndex. I don't see why your use would be against the norm.

Update: OK I see you have already changed your patch to make the feature enabled by default.
As far as the performance is concerned, I will have a go at improving it and hope to have something to share next week. How does that sound?

In D108560#3168006, @ymandel wrote:

In D108560#3167847, @salman-javed-nz wrote:

With regards to reverting it, is the cat out of the bag? I already see some usage of the NOLINTBEGIN feature in other projects.
There's also a NOLINT check globbing feature that builds on top of this, so it's not as simple as just backing out one commit.

If this is the approach we take, I prefer the logic to be switched around: the NOLINT block feature should be enabled by default. Once (the worst of) this performance issue is resolved, it would be nice from a usability perspective if the user did not have to explicitly opt in to use the feature.

In D108560#3167883, @aaron.ballman wrote:

In D108560#3167847, @salman-javed-nz wrote:

If the current code stayed in the main repo, how long could you afford to wait for me to provide a patch?

I would say that for me personally, at a maximum, we have to either fix or revert before this goes out in a release of Clang (because Clang 13 did not have support for NOLINTBEGIN/NOLINTEND, so this is technically unreleased functionality).

In D108560#3167847, @salman-javed-nz wrote:

I can have a go at coming up with a solution where it does caching of the NOLINT marker positions when a diagnostic is generated in a file. That would probably help a lot with the performance. It wouldn't be too complicated to implement either. Would you be happy with something like that?

Thanks for the investigation. Just exploring the options...

Will aim to review and come back to you in a couple of days.

Update "iff" comment based on review.

Nothing else that comes to mind that I want to see.
@Quuxplusone are you OK with the latest round of diffs?

I think the primary goal is satisfied - in all cases the cast is identified and a warning is generated.

LGTM, thanks!

Thanks for the patch.

What would you say are the key differences between this patch differ and previous attempts, e.g. https://reviews.llvm.org/D72566? How does this patch address the concerns raised in the previous reviews?

I reverted my changes to do with the invalid character substitution. Doing something akin to isAllowedInitiallyIDChar() and isAllowedIDChar() in Lexer.cpp will require converting from char* to UTF32*. Windows complicates things by using its own code page (typically Windows-1252). Will require a lot of careful consideration to implement correctly.

Back out the changes to the "replace invalid characters in an identifier with underscores" feature.
Making this feature work with Unicode characters on different operating systems significantly increases the amount of code change required.
This can always be revisited in another patch.