[PATCH] D116443: [clangd] Implement textDocument/typeDefinition

[Sam McCall via Phabricator via cfe-commits]

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


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1883
+      QualType VisitIfStmt(const IfStmt *S) { return type(S->getCond()); }
+      QualType VisitCaseStmt(const CaseStmt *S) { return type(S->getLHS()); }
+      QualType VisitCXXForRangeStmt(const CXXForRangeStmt *S) {
----------------
usaxena95 wrote:
> I think this would be useful for enumerations primarily. Would it work as of now (would we return the enum definition loc) ?
Yes, it does work (tested manually).


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1925-1927
+    // If we targeted something function-like, prefer its return type.
+    if (auto FT = Type->getAs<FunctionType>())
+      Type = FT->getReturnType();
----------------
usaxena95 wrote:
> Can this be accommodated in `typeForNode` ?
> It would be nicer if we keep this method only for plumbing.
Agree. It's not convenient to do it exactly in typeForNode though, as we have to wrap every return with the unwrapping logic.
I added `unwrapFindType` to do this, and that's a natural place to fix array/pointer cases too while here.


================
Comment at: clang-tools-extra/clangd/XRefs.h:110
+///
+/// For example, given `foo(b^ar())` where bar() returns unique_ptr<Foo>,
+/// this should return the definition of class Foo.
----------------
usaxena95 wrote:
> This is sounds confusing to me about the current behaviour. 
> We would return `unique_ptr<T>` here atm. Maybe just have simple `returns Foo` here as of now.
Whoops, you're right. I'd love to have this behavior but decided not to bite it off in this patch.


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:1805
+           "namespace m { Target tgt; } auto x = m^::tgt;",
+           "Target funcCall(); auto x = ^funcCall();",
+           "Aggregate a = { {}, ^{} };",
----------------
usaxena95 wrote:
> Can you add a lambda as well
> `"au^to x = []() -> Target {return Target{};}"`
Done. This isn't handled yet (lambdas are CXXRecordType, not FunctionType) so added it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116443