[PATCH] D48290: [clangd] Use workspace root path as hint path for resolving URIs in workspace/symbol

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/ClangdServer.cpp:119
+  auto FS = FSProvider.getFileSystem();
+  auto Status = FS->status(RootPath);
+  if (!Status)
----------------
why validate it?


================
Comment at: clangd/FindSymbols.h:30
 /// \p Limit limits the number of results returned (0 means no limit).
+/// \p RootPath Root path of the workspace. This is used as the hint path when
+/// resolving URIs.
----------------
Change to HintPath? There's no reason to couple this code to the fact that it's the workspace root in clangdserver.


================
Comment at: clangd/FindSymbols.h:30
 /// \p Limit limits the number of results returned (0 means no limit).
+/// \p RootPath Root path of the workspace. This is used as the hint path when
+/// resolving URIs.
----------------
sammccall wrote:
> Change to HintPath? There's no reason to couple this code to the fact that it's the workspace root in clangdserver.
you sometimes pass "" here, so we should document the semantics of that


================
Comment at: unittests/clangd/TestFS.cpp:70
+/// unittest: is a scheme that refers to files relative to testRoot().
+/// URI body is relative path to testRoot().
 class TestScheme : public URIScheme {
----------------
is a path relative to testRoot()


================
Comment at: unittests/clangd/TestFS.cpp:93
     return URI(Scheme, /*Authority=*/"",
-               llvm::sys::path::convert_to_slash(Body));
+               StringRef(llvm::sys::path::convert_to_slash(Body)).ltrim('/'));
   }
----------------
why this change?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48290