[PATCH] D80296: [clangd] Don't traverse the AST within uninteresting files during indexing

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz marked an inline comment as done.
adamcz added a comment.

I have no objections to landing this as-is, even if it represents a regression for some evil codebases.

A possible solution is to iterate through all the includes in a given file (SourceManager knows that already) and if any of them are outside of preamble, do not use this optimization on that file. Should be easy and, hopefully, fast. Most files are well-behaved and have includes in preamble only, thus the optimization would still kick in most of the time.



================
Comment at: clang-tools-extra/clangd/unittests/IndexActionTests.cpp:22
 using ::testing::ElementsAre;
+using testing::EndsWith;
 using ::testing::Not;
----------------
sammccall wrote:
> kadircet wrote:
> > nit `using ::testing::EndsWith`
> > 
> > was this `add using` tweak ?
> Yes, it was.
> I feel a bit silly as I'm well aware of the behaviour here and have argued it's the right thing, and wrote `testing::EndsWith` inline, extracted it, and didn't check the output (as it was offscreen).
> 
> @adamcz FYI
> If we see this a lot maybe we should consider something like "if all existing usings in the block are globally qualified...". Current behavior is definitely defensible though.
Agreed. I'm starting to think we should have some sort of config file that defines style for the codebase (beyond clang-format config, things like always-use-::-for-using or never-add-using-for-::std) and possibly guess the values, when missing, during background indexing. It could simplify code edits a lot.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80296/new/

https://reviews.llvm.org/D80296