LG with a couple of nits.

Looks good. Thanks for the fix! I'll get it landed for you.

In D97288#2581908, @sammccall wrote:

How strong is the need for this?
This adds complexity to a widely implemented and used interface, and the combination of virtual + default parameters can be at least a little confusing.

LG. Thanks for the fix!

Artem, could you set the repository to rG LLVM Github Monorepo when uploading patches as mentioned in https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface ? This way you'll allow pre-merge checks to run.

Thanks, looks good!

Apologies again for the long delay.

Thanks! Looks good now.

Looks good!

Looks good!

Trying to upload complete patch.

Manually cleaned up suboptimal fixes.

Thanks for the prompt fix, btw!

This fixes the issue with exponential traversal times for deeply nested lambdas. Please add a test though. For example, this one:

In D93688#2525947, @alexfh wrote:

This patch causes practically infinite traversal times on code that contains deeply nested lambdas. I'll try to get a suitable repro, but could you maybe revert this in the meantime?

This patch causes practically infinite traversal times on code that contains deeply nested lambdas. I'll try to get a suitable repro, but could you maybe revert this in the meantime?

LG

This is quite a bit of extra boilerplate, but it results in a more transparent error handling. However, the documentation would need to be changed (clang/docs/LibASTMatchersTutorial.rst). And what about other tools (e.g. clang-tools-extra/clang-include-fixer/tool/ClangIncludeFixer.cpp, clang-tools-extra/clang-include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp, etc.)?

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.