[PATCH] D130041: [clangd] Add decl/def support to SymbolDetails

[David Goldman via Phabricator via cfe-commits]

dgoldman added inline comments.


================
Comment at: clang-tools-extra/clangd/AST.cpp:174
   auto L = D.getLocation();
+  // Use the start of the first selector fragment instead of the -/+ location.
+  if (const auto *MD = dyn_cast<ObjCMethodDecl>(&D))
----------------
kadircet wrote:
> can we do this in a separate change? as it has wider implications
Sent out https://reviews.llvm.org/D130095, will rebase this on top once that's submitted.


================
Comment at: clang-tools-extra/clangd/unittests/SymbolInfoTests.cpp:29
+
+  const char *DeclMarker = nullptr;
+  const char *DefMarker = nullptr;
----------------
kadircet wrote:
> since you're always calling these `decl` or `def`, you can instead check whether the annotation has any range named `def`/`decl` and expect those accordingly rather than mentioning them one extra time inside the testcases. i.e:
> 
> case:
> 
> ```
> void $def[[foo]]() {}
>           int bar() {
>             fo^o();
>           }
> ```
> 
> check:
> ```
> Annotations Test(Case);
> SymbolDetails Expected;
> auto Def = Test.ranges("def");
> auto Decl = Test.ranges("decl");
> if (!Def.empty()) {
>   Expected.decl = Expected.def = makelocation ...
> } else if (!Decl.empty()) {
>   Expected.decl = makelocation ...
> }
> ```
Thanks, although that won't work for the `// Multiple symbols returned - using overloaded function name` test case. Should I move that one out to to be handled separately?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130041