[PATCH] D94785: [clangd] Index local classes, virtual and overriding methods.

[Haojian Wu via Phabricator via cfe-commits]

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