kadircet wrote: > 1. we no longer use SymbolOrigin::StdLib for collected symbols
thanks for catching that, I think that's OK to pass in an extra `SymbolOrigin` flag here. > 2. we run the symbol collector with CollectMainFileSymbols = true I think this is WAI. Firstly we shouldn't have (m)any symbols in main-file when running stdlib indexing (as we just stitch together a file that `#include <...>` all stdlib headers we know of). but even if we run into some, these symbols are all part of the preamble section. hence even-if they're main-file only, they should get the same treatment as any other preamble symbol. This also ensures we don't get different behavior if `<vector>` is indexed through preamble-index vs stdlib-index first. > 3. we use the indexing option SystemSymbolFilterKind::DeclarationsOnly agreed that this sounds like an improvement. > 4. we no longer use > [this](https://searchfox.org/llvm/rev/bb21a6819b3fb9d689de776f7ee768030dfbacea/clang-tools-extra/clangd/index/IndexAction.cpp#143-148) > "deeply nested" optimization I think this was just an oversight, we should probably have that in `indexSymbols` too. This can be a separate patch though. > If that seems preferable to you over adding a parameter to > createStaticIndexingAction Just to be clear, my concern is not about adding more knobs to `createStaticIndexingAction`, but rather it being the wrong fix. As your analysis revealed we actually had many more discrepancies between dynamic-index and stdlib-index. Amending `createStaticIndexingAction` to look more like dynamic-index will just blur the line between static and dynamic index, which sounds undesired overall. > I'm happy to revise the patch along these lines. That would be great, thanks a lot for taking care of this! https://github.com/llvm/llvm-project/pull/133681 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
