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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
