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