[PATCH] D52078: [clangd] Store preamble macros in dynamic index.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

This is definitely the right thing to do, thanks for finding it!
I've got a long comment about how everything used to be simpler in the good old days :-) I'm itching to refactor a bit, but land this first.



================
Comment at: clangd/index/FileIndex.cpp:34
   CollectorOpts.CountReferences = false;
+  CollectorOpts.CollectMacro = true;
   if (!URISchemes.empty())
----------------
I don't understand this change. Don't we also want this only if IndexMacros is true?
(It's possible it doesn't make a difference due to implementation details but it seems worth being clear here)


================
Comment at: clangd/index/FileIndex.h:93
 std::pair<SymbolSlab, RefSlab>
 indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
+         bool IndexMacros = false,
----------------
I'm concerned about the proliferation of parameters here. (Not new with this patch, it's been a continuous process - the first version had one input and one output!)
It's heading in the direction of being a catch-all "collect some data from an AST" function, at which point each caller has to think hard about every option and we're going to end up with bugs.
(For example: TestTU::index() passes "false" for IndexMacros. It seems to me maybe it should be "true". But it's really hard to tell.)
That's also pretty similar to the mission of SymbolCollector itself, so we're going to get some confusion there.

As far as I can tell, there's now two types of indexing that we do:
  - preamble indexing: we look at all decls, and only index those outside the main file. We index macros. We don't collect references. Inputs are ASTContext and PP.
  - mainfile-style indexing: we look only at top-level decls in the main file. We don't index macros. We collect references from the main file. Inputs are ParsedAST.
This really looks like it should be 2 functions with 2 and 1 parameters, rather than 1 function with 4.
Then callers will have two styles of indexing (with names!) to choose between, rather than being required to design their own. And we can get rid of the "is this a main-file index?" hacks in the implementation.

Sorry for jumping on you here, this change isn't any worse than the three previous patches that added knobs to this function.
This doesn't need to be addressed in this patch - could be before or after, and I'm happy to take this on myself. But I think we shouldn't kick this can down the road too much further, eventually we end up with APIs like clang :-)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078