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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: unittests/clangd/FindSymbolsTests.cpp:442
+                                    SymNameRange(Main.range("decl"))))),
+                          AllOf(WithName("f"), WithKind(SymbolKind::Method),
+                                SymNameRange(Main.range("def")))));
----------------
sammccall wrote:
> 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!)
Oh, yeah, that I agree with! Will look into fixing this in a follow-up change.


================
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:
> 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)
Yeah, you can. It's especially common with function where specializations don't name any arguments and those are inferred from the function type...


================
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:
> 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)
> 
Yeah, this fell out of the implementation of walking over the "user-written" part of the AST and it looked sensible so I updated the test.


================
Comment at: unittests/clangd/FindSymbolsTests.cpp:571
   addFile(FilePath, R"(
       enum {
         Red
----------------
sammccall wrote:
> 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.
I'm sold on the consistency argument. Besides, the anonymous enums are not too frequent anyway, so treating them in a special way might be an overkill.
We can always tweak this in the future.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52311