[PATCH] D116630: [clangd] Handle declarators more consistently in Selection.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 3 inline comments as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Selection.cpp:757
     if (const auto *TL = N.get<TypeLoc>()) {
-      // e.g. EltType Foo[OuterSize][InnerSize];
-      //      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ArrayTypeLoc (Outer)
----------------
hokein wrote:
> I'd prefer to keep this comment, while we have some high-level and abstract comment in the new patch, I think it is still useful to give us some details (otherwise, I'd probably forget everything when coming back to read the code a few months later).
> 
> 
> I also want to add the example `int (*Fun(OuterType))(InnerType);` from D116618 here, I understand it might be too verbose (personally, I find it useful to understand this part of code), up to you.
Yeah, the example is useful.
My problem was that the examples were showing how each declarator type combined with itself, which was both confusing (as you have to describe which you're talking about) and didn't cover the general case.

I've added a new example, which uses multiple different types of declarators and also shows nontrivial non-declarator types for comparison.


================
Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:209
           )cpp",
-          "CXXConstructExpr",
+          "VarDecl",
       },
----------------
hokein wrote:
> Maybe a FIXME for "CXXConstructExpr is better", but I don't come up with a good fix.
I actually like the way we treat CXXConstructExpr, which is that it owns the brackets and nothing else.
And having VarDecl consistently own the = is nice too.

This is a divergence from the AST so I've left a comment, but I think it's actually good.

(As mentioned offline: the CXXConstructExpr in this case isn't actually the interesting constructor, it's the elidable move construction, so this doesn't really hurt go-to-definition etc)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116630/new/

https://reviews.llvm.org/D116630