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

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 accepted this revision.
usaxena95 added a comment.
This revision is now accepted and ready to land.

Thanks. LGTM.



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1869
+      QualType VisitDesignatedInitExpr(const DesignatedInitExpr *S) {
+        for (auto &D : llvm::reverse(S->designators()))
+          if (D.isFieldDesignator())
----------------
Reason for `reverse` isn't clear to me. Can you add a comment.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1880-1882
+      QualType VisitWhileStmt(const WhileStmt *S) { return type(S->getCond()); }
+      QualType VisitDoStmt(const DoStmt *S) { return type(S->getCond()); }
+      QualType VisitIfStmt(const IfStmt *S) { return type(S->getCond()); }
----------------
Can you add tests for these as well.


================
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) {
----------------
I think this would be useful for enumerations primarily. Would it work as of now (would we return the enum definition loc) ?


================
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();
----------------
Can this be accommodated in `typeForNode` ?
It would be nicer if we keep this method only for plumbing.


================
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.
----------------
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.


================
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 = { {}, ^{} };",
----------------
Can you add a lambda as well
`"au^to x = []() -> Target {return Target{};}"`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116443