hokein added a comment. In D94785#2503939 <https://reviews.llvm.org/D94785#2503939>, @nridge wrote:
> In D94785#2501806 <https://reviews.llvm.org/D94785#2501806>, @usaxena95 wrote: > >> Although now that I enabled this in all SymbolCollectorTests, this causes a >> regression in RefContainers. >> >> void f2() { >> (void) $ref1a[[f1]](1); >> auto fptr = &$ref1b[[f1]]; >> } >> >> `&f1` is contained in `fptr` instead of `f2`. No immediate ideas why would >> this happen. > > I have a theory as to why. > > If you look at the next test case in `RefContainers`: > > int $toplevel2[[v1]] = $ref2[[f1]](2); > > here, `v1` is a global variable, and it is the containing symbol for the > reference in the initializer. > > So, I think libIndex considers references in initializers to be contained in > the declaration of the variable. The reason this didn't happen for local > variables until now, is that `IndexFunctionLocals` was false, and libIndex > probably avoids returning non-indexed symbols in `ASTNode::Parent`. This looks like a bug in libindex -- not sure we should fix that before this patch, I think we can probably live with this, and fix the bug afterwards. ================ Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:64 index::IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly; - IndexOpts.IndexFunctionLocals = false; + IndexOpts.IndexFunctionLocals = true; if (IsIndexMainAST) { ---------------- Can you add a comment explicitly mentioning that we only index function-local classes and its methods? Without a comment, the reader would expect we index all function-local symbols by just reading this part of code. ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:223 // Skip symbols in anonymous namespaces in header files. if (!IsMainFileOnly && ND.isInAnonymousNamespace()) return false; ---------------- nit: CollectMainFileSymbols affects the function-local symbols, I think? please update the comments of `SymbolCollector::Options::CollectMainFileSymbols`. ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:231 + // For function local symbols, index only classes. + if (ND.getParentFunctionOrMethod()) + return isa<RecordDecl>(ND); ---------------- nit: use `index::isFunctionLocalSymbol` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94785/new/ https://reviews.llvm.org/D94785 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
