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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
