[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/FindSymbols.cpp:190
+  index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
+  SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
----------------
sammccall wrote:
> may want to add a FIXME here: per the tests, it's not classifying constructor/destructor/operator correctly (they're all "methods").
> 
> cons/dest is just indexSymbolKindToSymbolKind needing an update, but operator isn't represented in index::SymbolKind.
> Maybe we should stop using index::SymbolKind entirely, it doesn't appear to be great.
Added a FIXME.
>From what I can tell, writing our own classification function shouldn't be too much work and would clearly fix those cases. We should do this!


================
Comment at: clangd/FindSymbols.cpp:208
+std::vector<DocumentSymbol> collectDocSymbols(ParsedAST &AST) {
+  struct CollectSymbols {
+    CollectSymbols(ParsedAST &AST, std::vector<DocumentSymbol> &Results)
----------------
sammccall wrote:
> sammccall wrote:
> > sammccall wrote:
> > > this class seems maybe too long to live inside this function. (I wouldn't worry about it being visible to other stuff, the file is small and focused)
> > (this looks more like a class than a struct?)
> should this be a RecursiveASTVisitor?
> I guess not, but please explain why (there's no documentation on the implementation strategy, and it's complicated)
Added a comment.


================
Comment at: clangd/FindSymbols.cpp:264
+        // ignored.
+        if (auto *Info = Func->getTemplateSpecializationInfo()) {
+          if (!Info->isExplicitInstantiationOrSpecialization())
----------------
sammccall wrote:
> isn't this covered by D->isImplicit?
Nope, IIUC things are only marked "implicit" in clangd if they were generated by the compiler from the start.
For this case, the **initial** template code was written by the user, so `isImplicit()` returns false for both the template and its instantiations.


================
Comment at: clangd/FindSymbols.cpp:276
+      //     children.
+      //   - implicit instantiations, i.e. not written by the user.
+      //     Do not visit at all, they are not present in the code.
----------------
sammccall wrote:
> isn't this covered by D->isImplicit() above?
See the other comment.


================
Comment at: clangd/Protocol.h:43
   InvalidParams = -32602,
+
   InternalError = -32603,
----------------
sammccall wrote:
> why?
Sorry, accidental change. Reverted.


================
Comment at: unittests/clangd/FindSymbolsTests.cpp:442
+                                    SymNameRange(Main.range("decl"))))),
+                          AllOf(WithName("f"), WithKind(SymbolKind::Method),
+                                SymNameRange(Main.range("def")))));
----------------
sammccall wrote:
> this one is to be fixed, right?
Why? The outline view gives both the declaration and the definition, since both were written in the code.
That seems to be in line with what I'd expect from the outline view.


================
Comment at: unittests/clangd/FindSymbolsTests.cpp:521
+                ChildrenAre(AllOf(WithName("x"), WithKind(SymbolKind::Field)))),
+          AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct),
+                ChildrenAre(WithName("y"))),
----------------
sammccall wrote:
> hmm, this seems pretty confusing - I think `Tmpl<float>` would be a clearer name for a specialization, even if we just have `Tmpl` for the primary template.
> Partial specializations are confusing, though :-/
Done. Now prints as `Tmpl<float>`.
This may also include some arguments not written by the users (e.g. some default args), but added a FIXME to fix this, it's not entirely trivial


================
Comment at: unittests/clangd/FindSymbolsTests.cpp:523
+                ChildrenAre(WithName("y"))),
+          AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct), NoChildren()),
+          AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct), NoChildren())));
----------------
sammccall wrote:
> why the change in policy with this patch? (one of these previously was deliberately not indexed, now is)
We might have different goals in mind here.
>From my perspective, the purpose of the outline is to cover all things written in the code: there are 4 decls that the user wrote: one primary template, one template  specialization and two template instantiations.

What is your model here?


================
Comment at: unittests/clangd/FindSymbolsTests.cpp:571
   addFile(FilePath, R"(
       enum {
         Red
----------------
sammccall wrote:
> hmm, our handling of anonymous enums seems different than anonymous namespaces - why?
No rigorous reasons. The namespaces tend to group more things together, so it's arguably more useful to have an option of folding their subitems in the tree.
Anonymous enums are typically used only in the classes (and on the top-level in C to define constants?) and I feel having an extra node there merely produces noise.
Happy to reconsider, what's your preferred behavior?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52311