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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: unittests/clangd/FindSymbolsTests.cpp:442
+                                    SymNameRange(Main.range("decl"))))),
+                          AllOf(WithName("f"), WithKind(SymbolKind::Method),
+                                SymNameRange(Main.range("def")))));
----------------
ilya-biryukov wrote:
> 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.
Sorry, I meant the name should be `Foo::def` instead of `def`, which you mentioned in a previous comment.

OK to land without this, but I think it deserves a fixme.
(If you think this is the *right* behavior, let's discuss further!)


================
Comment at: unittests/clangd/FindSymbolsTests.cpp:521
+                ChildrenAre(AllOf(WithName("x"), WithKind(SymbolKind::Field)))),
+          AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct),
+                ChildrenAre(WithName("y"))),
----------------
ilya-biryukov wrote:
> 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
Nice! I thought this was going to be really hard.

(You can specialize without naming the defaulted args? That sounds... obscure. Definitely fine to leave for later)


================
Comment at: unittests/clangd/FindSymbolsTests.cpp:523
+                ChildrenAre(WithName("y"))),
+          AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct), NoChildren()),
+          AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct), NoChildren())));
----------------
ilya-biryukov wrote:
> 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?
I don't have a particular opinion, just wondering if this was a deliberate change or fell out of the implementation.

(The old behavior was tested, so I guess Marc-Andre had a reason, but it didn't come up in the review. I don't think this is common enough to worry much about either way)



================
Comment at: unittests/clangd/FindSymbolsTests.cpp:571
   addFile(FilePath, R"(
       enum {
         Red
----------------
ilya-biryukov wrote:
> 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?
I *think* I might prefer the opposite - namespaces flat and enums grouped :-)
I'm not certain nor adamant about either, so consider my thoughts below, and pick something.

**Namespaces**: anonymous namespaces are mostly (entirely?) used to control visibility. I don't think they *group* in a meaningful way, they just hide things like `static` does. In a perfect world I think there'd be a "private" bit on outline elements that was shown in the UI, and we'd put it on both things-in-anon-namespaces and static-as-in-private decls. The main counterargument I can see: because we typically don't indent inside namespaces, the enclosing anonymous namespace can be hard to recognize and jump to.

**Enums**: conversely, I think these _typically_ do group their elements, and there's often a strong semantic boundary between the last entry in the enum and the first decl after it. When people use a namespacing prefix (C-style VK_Foo, VK_Bar) then it's visually noticeable, but this is far from universal in C++ (particularly at class scope).

**Consistency**: It's not *really* confusing if these are different, just *slightly* confusing. Anonymous structs etc must certainly be grouped, so this is a (weak) argument for grouping everything.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52311