[PATCH] D82938: [clangd] Implement path and URI translation for remote index

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Rest is just nits, thanks!



================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:38
+/// provided by the client.
+llvm::Optional<std::string> relativePathToURI(llvm::StringRef RelativePath,
+                                              llvm::StringRef IndexRoot) {
----------------
these functions are nice. It might be clearer to expose them and test them directly, particularly for error cases. Up to you.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:39
+llvm::Optional<std::string> relativePathToURI(llvm::StringRef RelativePath,
+                                              llvm::StringRef IndexRoot) {
+  assert(RelativePath == llvm::sys::path::convert_to_slash(
----------------
can we add an assert that the index root is valid? (ends with a platform-appropriate slash)


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:45
+  if (llvm::sys::path::is_absolute(RelativePath)) {
+    elog("{0} is not relative path", RelativePath);
+    return llvm::None;
----------------
message needs a bit more context. `Remote index client got absolute path from server: {0}`?


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:50
+  llvm::sys::path::append(FullPath, RelativePath);
+  const auto Result = URI::createFile(FullPath);
+  return Result.toString();
----------------
nit: lots of `const auto` throughout - we tend not to use `const` for local values (as opposed to references or pointers) so this is a bit confusing, particularly when combined with auto. Up to you though.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:57
+llvm::Optional<std::string> uriToRelativePath(llvm::StringRef URI,
+                                              llvm::StringRef IndexRoot) {
+  auto ParsedURI = URI::parse(URI);
----------------
can we add an assert that IndexRoot is valid?


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:60
+  if (!ParsedURI) {
+    elog("Can not parse URI {0}: {1}", URI, ParsedURI.takeError());
+    return llvm::None;
----------------
Similarly: `Remote index got bad URI from client: ...`


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:64
+  if (ParsedURI->scheme() != "file") {
+    elog("Can not parse URI with scheme other than \"file\" {0}", URI);
+    return llvm::None;
----------------
I think it's fine to fold this into the previous check: "bad URI" covers it


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:68
+  llvm::SmallString<256> Result = ParsedURI->body();
+  if (IndexRoot.empty())
+    return std::string(Result);
----------------
This isn't a valid IndexRoot, we should assert rather than handle this case I think.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:131
+  if (!RelativePath)
+    return Result;
+  *Result.mutable_file_path() = *RelativePath;
----------------
shouldn't this return llvm::None, too? a SymbolLocation isn't valid without a path.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:145
+  // If header starts with < or " it is not a URI: do not attempt to parse it.
+  if (Header.front() == '<' || Header.front() == '"') {
+    Result.set_header(Header);
----------------
nit: isLiteralInclude in Headers.h (sorry, had forgotten about this func).
Calling that function probably avoids the need for the comment too.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:149
+  }
+  auto URI = URI::parse(Header);
+  if (!URI) {
----------------
isn't the rest of this uriToRelativePath?


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:210
   if (!ID) {
-    elog("Cannot convert parse SymbolID {} from Protobuf: {}", ID.takeError(),
+    elog("Cannot parse SymbolID {} given Protobuf: {}", ID.takeError(),
          Message.ShortDebugString());
----------------
{} with no index is a nice idea but I think not actually supported


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h:19
+// path and passes paths relative to the project root over the wire
+// ("include/HelloWorld.h" in this example). The indexed project root is passed
+// passed to the remote server. It contains absolute path to the project root
----------------
nit "is passed passed to the remote server".


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h:20
+// ("include/HelloWorld.h" in this example). The indexed project root is passed
+// passed to the remote server. It contains absolute path to the project root
+// and includes a trailing slash. Relative path passed over the wire has unix
----------------
"It contains absolute path to the project root... relative path has unix slashes" 

can we pull this out into a separate paragraph at the end? The rest is describing the flow, and this describes an API detail.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h:20
+// ("include/HelloWorld.h" in this example). The indexed project root is passed
+// passed to the remote server. It contains absolute path to the project root
+// and includes a trailing slash. Relative path passed over the wire has unix
----------------
sammccall wrote:
> "It contains absolute path to the project root... relative path has unix slashes" 
> 
> can we pull this out into a separate paragraph at the end? The rest is describing the flow, and this describes an API detail.
nit: "*the* absolute path", "*the* relative path" etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82938