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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/index/FileIndex.h:93
 std::pair<SymbolSlab, RefSlab>
 indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
+         bool IndexMacros = false,
----------------
sammccall wrote:
> 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 :-)
I'm definitely on board with this and happy to do the refactoring before landing this patch, to break API just once ;)  Just to clarify my understanding before doing that, do we also want to split `FileIndex::update` into `updatePreamble` and `updateMain`?  And the new `indexAST` will be `indexMain` and `indexPreamble`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52078