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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1528
   }
-
+  auto MainFilePath =
+      getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
----------------
dgoldman wrote:
> dgoldman wrote:
> > sammccall wrote:
> > > kadircet wrote:
> > > > let's just pass the TUPath from ClangdServer into the request. (we should do the same for `locateSymbolAt`, but on a separate change)
> > > driveby: @kadircet any reason not to store the TUPath on ParsedAST? We already pass it to ParsedAST::build()
> > I think this would simplify things quite a bit - otherwise I also need to update the DumpSymbol tweak to store/compute the TUPath (or just provide an empty one). LMK what I should do.
> Also, not sure how this would affect the Windows failure, which currently fails since the decl/definition locations reference "file:///C:/clangd-test/simple.cpp" instead of "test:///simple.cpp"... could add a regex or limit the test to non-Windows as a work around though. "textDocument/publishDiagnostics" also seems to reference that absolute path as well instead of the test:// URI
sent out D130690 for preserving TUPath in ParsedAST.

as for matching output in lit tests, clangd always outputs file uris. so you should rather match with a regex (unless testing something platform-specific).
you can use `# CHECK-NEXT:      "uri": "file://{{.*}}/clangd-test/simple.cpp"` instead to match output.


================
Comment at: clang-tools-extra/clangd/unittests/SymbolInfoTests.cpp:29
+
+  const char *DeclMarker = nullptr;
+  const char *DefMarker = nullptr;
----------------
dgoldman wrote:
> 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?
ah sorry i missed those, nvm this one. no need for splitting those out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130041