Address review comments.

Also rebase on top of main.

Address review comments: the structure is a bit different but the bug is now
actually removed.

Remove redundant comment.

Remove unwanted formatting changes.

In D125088#3499649, @nridge wrote:

Is there a related change that explains what this setting does?

Resolve comment.

Resolve the comments.

Use strlen for performance.

Remove unused include.

Thanks, this looks good; just few nits regarding the comments.

Resolve comments.

@sammccall ping

Oops, sorry, I linked the wrong revision; here's the prototype: we plan to start rolling it out gradually https://reviews.llvm.org/D122677 and then have a common library that we could share between both clangd and Clang-Tidy (and potentially other tools).

In D123488#3444900, @dsanders11 wrote:

Cool stuff! Chiming in randomly here, but I've been keeping a loose eye on the progress of IncludeCleaner.

I've been using IncludeCleaner to power include cleanup in the Chromium code base for the past few months, and have built Python scripts around it so I can more efficiently use it at a code base level. You can find all that fun stuff on my repo: https://github.com/dsanders11/chromium-include-cleanup

In my usages, I've found there are three categories of ignores I need (you can see my ongoing list of them in the config file: https://github.com/dsanders11/chromium-include-cleanup/blob/main/configs/chromium.json):

• Skip over a file entirely and don't provide any diagnostics for it. This is useful to ignore umbrella headers, without needing to ignore the headers included in that umbrella header.
• Ignore diagnostics for a particular header, which is what is implemented here so far.
• Ignore an include edge (filename, header). This is useful for cases where it's a spurious diagnostic, like when the usage is inside of a macro definition, so IncludeCleaner thinks it is unused but it is not.

This isn't entirely applicable here, but could be insightful: I initially I had my filtering mechanism in the script which ran clangd over the source files, but I found that it was better for my needs to decouple the filtering from the generating the diagnostics. Main reason being that generating the diagnostics for all 80k files in the Chromium code base takes hours to run, so if I found some new ignores I wanted to add, I'd have to do another 7 hour run (on the hardware I'm using), which wasn't efficient. So I let the many script output all diagnostics, and then I filter after the fact, letting me tweak the filtering without needing to re-run the whole thing. So for my own needs I'll probably stick with doing the filtering after the fact, even if it's built-in. Not too applicable for IncludeCleaner usage through say VS Code, but there are different challenges when doing it at code base level.

Fix the comment.

Resolve comments.

LG to me with the comments that Haojian made!

I think this is no longer [WIP] but rather review-ready, right?

In D123031#3429801, @thakis wrote:

Looks like this breaks tests on Windows: http://45.33.8.238/win/55690/step_9.txt

please take a look, etc :)

Fix the test case.

Address review comments.

Rebase correctly.

Rebase on top of main.

Switch to stable file UniqueIDs.

In D123031#3426382, @kadircet wrote:

Right! I think I also need to do that in SymbolCollector.cpp.

Now that you mentioned this too, it feels like the issue is not about being consistent, as I think all of the places that we have today are actually making use of FileEntry::getName as keys to CanonicalIncludes::{addMapping,mapHeader}. Are we sure we're not relying on some sort of canonization of file path (AFAICT, that's not the case the only canonization that's happening is dropping ..) when you use tryGetRealPathName instead of getName?

In D123031#3426303, @kadircet wrote:

note that you seem to be using auto PublicHeader = CanonIncludes.mapHeader(Entry->getName()); (i.e. getName) in D120306 and not tryGetRealPathName. it might be nice to have a test case here, at least one that only fails/runs on windows

This is required for D120306 which was landed but later reverted because of the failing Windows tests (different slash types for filenames requested during preamble parsing and in the main file).

Fix the behavior: get Real Path instead.

Address review comments.

Use better name for header recorder lambda.

Add some docs, rebase on top of main.

Fix a bug, add tests for diagnostics. This is ready for a review now.

Switch to a different model of recording includes that differentiates between
user includes and public headers recored through IWYU pragmas.

Hi! I'm glad you're excited about IncludeCleaner and having a tool to try it out seems like a cool idea!

In D120306#3337374, @sammccall wrote:

In D120306#3337288, @kbobyrev wrote:

In D120306#3337212, @sammccall wrote:

What's the policy this patch intends to implement?

I'm a little concerned by building up maps of filenames with segment limits - it looks like some kind of heuristic/partial matching.

Basically, handling the mappings that. IWYU pragmas provide.

Yes, I'm asking you to describe that handling in a couple of sentences so I can understand what you intend it to be.

e.g. for includecleaner in general, the approach is: walk the AST, map each node to decls it uses, map decls to locations, map locations to files, then check which includes point to files that were not identified.

Move suffix map to CanonicalIncludes (where it logically belongs), remove
component limit there.

In D120306#3337212, @sammccall wrote:

What's the policy this patch intends to implement?

I'm a little concerned by building up maps of filenames with segment limits - it looks like some kind of heuristic/partial matching.

The patch is working but I didn't add tests yet. Also, I think I should move the suffix mapping to CanonicalIncludes structure rather than Headers.

In D118847#3307497, @Febbe wrote:

Can I still add a diff, or does this cause a revoke (apply the rest of the feedback)?
Also, is the commit added automatically to the repo, or do I / another one have to rebase it.

Sorry for those questions, this is my first contribution to llvm.

LG, thanks!

Mostly LG with few nits. I think it might be able to make it into the 14 release if we cherry-pick into the release branch.

In D119130#3307333, @paulwalker-arm wrote:

After this commit I am seeing the link time error

ld.lld: error: undefined symbol: clang::DeclContext::isInlineNamespace() const
>>> referenced by StandardLibrary.cpp:118 (/home/pmw/projects/upstream-llvm/llvm-project/clang/lib/Tooling/Inclusions/StandardLibrary.cpp:118)
>>>               tools/clang/lib/Tooling/Inclusions/CMakeFiles/obj.clangToolingInclusions.dir/StandardLibrary.cpp.o:(clang::tooling::stdlib::Recognizer::namespaceSymbols(clang::NamespaceDecl const*)::'lambda'()::operator()() const)

ld.lld: error: undefined symbol: clang::Decl::castFromDeclContext(clang::DeclContext const*)
>>> referenced by DeclBase.h:2562 (/home/pmw/projects/upstream-llvm/llvm-project/clang/include/clang/AST/DeclBase.h:2562)
>>>               tools/clang/lib/Tooling/Inclusions/CMakeFiles/obj.clangToolingInclusions.dir/StandardLibrary.cpp.o:(clang::cast_convert_decl_context<clang::Decl, false>::doit(clang::DeclContext*))
>>> referenced by DeclBase.h:2558 (/home/pmw/projects/upstream-llvm/llvm-project/clang/include/clang/AST/DeclBase.h:2558)
>>>               tools/clang/lib/Tooling/Inclusions/CMakeFiles/obj.clangToolingInclusions.dir/StandardLibrary.cpp.o:(clang::cast_convert_decl_context<clang::NamedDecl, false>::doit(clang::DeclContext const*))

If I revert this commit it goes away. I've tried a fresh cmake and build and get the same result.