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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks, this LG per symboldetails change but as discussed offline i think we actually need a new request or a capability to indicate this (and having a new request is just easier for both clients and clangd) otherwise it can't be reliably used by clients.

in addition to that let's move the objcmethod related changes to its own patch since it's addressing a different issue.



================
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))
----------------
can we do this in a separate change? as it has wider implications


================
Comment at: clang-tools-extra/clangd/Protocol.h:1097
+
+  llvm::Optional<Location> declarationRange;
+
----------------
ah getting absolute paths .. alright i guess we need to make this one optional, sorry for the noise on this one :/


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:86
       return RD->getDefinition();
+  if (const auto *MD = dyn_cast<ObjCMethodDecl>(D))
+    if (MD->isThisDeclarationADefinition())
----------------
again let's move this into a separate change


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1528
   }
-
+  auto MainFilePath =
+      getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
----------------
let's just pass the TUPath from ClangdServer into the request. (we should do the same for `locateSymbolAt`, but on a separate change)


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1557
+            AST.getASTContext(), nameLocation(*Def, SM), *MainFilePath);
+      const NamedDecl *Decl = getPreferredDecl(D);
+      NewSymbol.declarationRange = makeLocation(
----------------
let's do this cannonicalization in the beginning of the loop, i.e:
```
for (const NamedDecl *D : getDeclAtPosition(AST, *CurLoc, Relations)) {
  D = getPreferredDecl(D);
  ....
}
```


================
Comment at: clang-tools-extra/clangd/test/symbol-info.test:4
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///simple.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
 ---
----------------
can you also introduce the definition?


================
Comment at: clang-tools-extra/clangd/unittests/SymbolInfoTests.cpp:24
+  std::string Name;
+
+  std::string Container;
----------------
nit: drop the empty lines in between these fields


================
Comment at: clang-tools-extra/clangd/unittests/SymbolInfoTests.cpp:29
+
+  const char *DeclMarker = nullptr;
+  const char *DefMarker = nullptr;
----------------
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 ...
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130041