https://github.com/HighCommander4 commented:
Nice, looks like the code changes here ended up being relatively straightforward. I haven't had a chance to take a detailed look at everything yet, but a few high-level comments: 1. I'm thinking we should keep the additional indexing we're doing conditional on `IndexingOptions::IndexImplicitInstantiation` (and turn on that flag for clangd). This limits the impact of the change on other libIndex consumers who haven't asked for implicit instantiations. 2. Since we're changing libIndex behaviour, we should technically include a libIndex test as well. It looks like there are two kinds, lit tests in `clang/test/Index` and unit tests in `clang/unittest/Index`. Most are lit tests, which seems to use a bespoke command line tool called `c-index-test` which exercises libIndex indirectly through the libclang API... yuck. The unit tests are nice and simple though, I'd suggest that (will leave it up to you though). 3. It may be worth trying to avoid a bit more work in `SymbolCollector` for the new instantiations, than just `CollectRef=false`. In particular, `addDeclaration` and `addDefinition` do things like computing documentation strings which seems like unfortunate duplication to do for implicit instantiations. https://github.com/llvm/llvm-project/pull/177273 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
