Pre-merge builder can't apply this patch: https://buildkite.com/llvm-project/diff-checks/builds/18651
Is it based on https://reviews.llvm.org/D91302 ? Do we need the intermediate state? Maybe squash the two patches together for simplicity?

Please fix the typo that results in a compile error.

Removed unrelated whitespace changes.

In D80499#2352339, @steveire wrote:

@alexfh This change is based on the behavior of AST Matchers being changed to ignore invisible/implicit AST nodes by default. As the default was not changed in the end, this patch would need to be updated to add traverse(TK_IgnoreUnlessSpelledInSource) wrapping around the matchers.

Before I update the patch to add that, do you have any feedback?

In D89886#2347376, @DmitryPolukhin wrote:

In D89886#2346851, @alexfh wrote:

Ah, btw, any chance of adding a test for this?

Oh, I was not able to create small reproducer that without including large Apple Frameworks with modules :( My hypothesis that it is side effect of module cache that triggers module load before it is referenced from sources. I tested it on reproducer from PR47839 + my real internal example.

Ah, btw, any chance of adding a test for this?

Looks good!

LG

Thanks for the fix! However, I'm not sure it's possible to correctly rewrite code in all cases where macros are involved. See a couple of motivating examples in the comment.

Sorry for the delay. This patch fell through the cracks. If you're still interested, could you rebase it on top of current HEAD and upload a full diff? Or use the arcanist tool, see https://llvm.org/docs/Phabricator.html.

Looks good!

Feel free to ping patches every week or so. It looks like in this case all the reviewers were swamped with something else at the time.

Looks good! Thanks for the fix! IIUC, this is related to https://bugs.llvm.org/show_bug.cgi?id=34879? Makes sense to specify this in the patch description.

• Updated release notes and default option values in docs

• clang-format
• addressed review comments

Looks good modulo comment.

LG with a couple of comments. Do you need someone to land the patch for you?

Thanks for the patch! Looks generally good. A few comments inline.

In D83223#2147306, @aaron.ballman wrote:

In D83223#2147247, @njames93 wrote:

In D83223#2147072, @aaron.ballman wrote:

 // This is not identified as a license comment as the
// block is followed by code.
void foo();

FWIW: https://github.com/GrammaTech/gtirb-pprinter/blob/master/include/gtirb_pprinter/AttPrettyPrinter.hpp or https://github.com/GrammaTech/gtirb/blob/master/include/gtirb/AuxData.hpp (so there are projects which do not put a newline between the license and code).

Short of creating an AI that understands context it won't be possible to determine the difference between license and general documentation, in any case I feel this heuristic is the safest way to ensure good coverage with minimised risk of inserting the guard in the middle of documentation,

My instinct is that we shouldn't be trying to play those games in the first place and should consider *all* leading comments and empty (whitespace-only) lines as part of the "license" and expect the first significant token to be the header guard. e.g., this isn't about the license at all, it's about whether you can have prose before the header guard or not. It's not uncommon for projects to put prose before header guards, nor is it uncommon for it to go after the header guards. tbh, that feels a bit like an option for the feature rather than an automatic behavior because I could also see a project wanting to enforce a consistent style.

Apologies for the delay! It's sort of a crazy time now =\

In D75184#1914788, @njames93 wrote:

In D75184#1914705, @DmitryPolukhin wrote:

You are absolutely right about current behaviour. Thank you for catching this odd behaviour. I'm not 100% confident what is the right behaviour but my guess is that overriding local option from parent config with global from child folder is better so I implemented it and added corresponding test.

It's a tricky one that, but I thing overriding the local option with the global in the sub directory is the correct way to go about this as well.

Internally we have something similar, but with unconditional inheritance and a way to include other configs. I was thinking about implementing this in the FileOptionsProvider, but decided that this would be an overkill for most real projects. Overall the patch looks good to me, but please update the documentation and add a test to ensure that check options get merged correctly.

In D75441#1901071, @gribozavr2 wrote:

I think my preference is to go with isLanguageVersionSupported() and not use base classes.

+1, I can see this working, but the introduction of new concepts does not seem like it pulls its weight given the tiny amount of boilerplate that they eliminate.

LG

Next step is to plumb this to clang-tidy.

In D69782#1886766, @compositeprimes wrote:

I don't have commit rights (this is my first llvm commit!), so could someone help out? Thanks!

LG

Do you have commit rights or should someone help you committing the patch?

In D72730#1866043, @AlexanderLanin wrote:

On second thought maybe this should be fixed in clang-tidy and not here?
Maybe besides -export-fixes there should be an -export-warnings or just -yaml-export?

A few nits.

Thanks for the update!

LG. Thanks for the fix!

In D72527#1843475, @Eugene.Zelenko wrote:

As I wrote, I don't have GitHub commit access, so I need help with commit.

In D72527#1843427, @Eugene.Zelenko wrote:

Just ping.

A drive-by comment.

How is this different from -Wmissing-prototypes?

LG with an outstanding comment.

In D72527#1817518, @Eugene.Zelenko wrote:

Both scripts works fine. However rename script should also sort entries alphabetically, but probably this should be separate patch.

LG with a nit, if this works.

Awesome! Thanks for updating the script! A few nits, otherwise LG, if this works.

What version of clang-tidy and clang-format does this run? Whatever is available in Debian packages?

This list used to be automatically updated by the clang-tools-extra/clang-tidy/add_new_check.py script. I suppose, this is broken now?

• Added a test with an empty attribute list.

In D70974#1769036, @gribozavr2 wrote:

In D70974#1768902, @aaron.ballman wrote:

In D70974#1768871, @gribozavr2 wrote:

I'm not convinced this feature is worth implementing at all, because there's a good alternative to a macro here -- a namespace alias. What is the reason to use a macro instead of a namespace alias?

While I think that's a superior solution to using macros, some users have macros instead. This fixes a bug reported in https://bugs.llvm.org/show_bug.cgi?id=26274 and I agree that the behavior described in that bug is not what I would expect it to be.

I mean, it is possible to break pretty much any ClangTidy check with sneaky code. But there's a limit to which we should try to make things work in tricky corner cases. For example, the fix in this patch does not handle function-like macros (namespace MY_LIBRARY_NAMESPACE_FOR_VERSION(42) {).

LGTM if we must... but I don't think we should.