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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
