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