[PATCH] D64305: [clangd] Add path mappings functionality

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this looks a lot better.
Main thing that was unclear to me is the fact that the paths in the mappings are now URI-paths, so "/C:/foo" on windows.

This looks like the right idea as it ensures much of the path-mapping code gets to ignore slashes and such.
Docs/naming could reflect it a bit better.



================
Comment at: clang-tools-extra/clangd/PathMapping.cpp:30
+  const auto &K = V.kind();
+  if (K == Kind::Object) {
+    for (auto &KV : *V.getAsObject()) {
----------------
wwagner19 wrote:
> sammccall wrote:
> > object keys may be file uris too. (see `WorkspaceEdit.changes`)
> > 
> > This case is going to be a bit annoying to handle, I think :-(
> Indeed, it seems so. It doesn't look like `json::Object` has any key removal functionality (although I could very well be reading this wrong). If so, then I guess I can just create a new `json::Object`, moving over the old values, and replacing the keys if necessary. 
Sorry, you're right - I'll fix `json::Object`.
Nevertheless I think the copy-into-new-object approach is the clearest way to deal with renames of keys that may otherwise collide in the intermediate state.


================
Comment at: clang-tools-extra/clangd/PathMapping.cpp:36
   using Kind = llvm::json::Value::Kind;
-  const auto &K = V.kind();
+  const Kind &K = V.kind();
   if (K == Kind::Object) {
----------------
by value, not by ref


================
Comment at: clang-tools-extra/clangd/PathMapping.cpp:137
 
+// Returns whether a path mapping should take place for \pOrigPath
+// i.e. \pMappingPath is a proper sub-path of \p OrigPath
----------------
this isn't a doxygen comment, please omit \p


================
Comment at: clang-tools-extra/clangd/PathMapping.cpp:153
+// Converts \pPath to a posix-style absolute, i.e. if it's a windows path
+// then the backward-slash notation will be converted to forward slash
+llvm::Expected<std::string> parsePath(llvm::StringRef Path) {
----------------
"posix-style" doesn't really describe representing `c:\foo` as `/c:/foo`. That's really *just* a URI thing AFAIK.

Something like "Converts a unix/windows path to the path part of a file URI".
But in that case, I think the implementation is just `URI::createFile(Path).body()`. Does that pass tests?


================
Comment at: clang-tools-extra/clangd/PathMapping.cpp:201
+                                          const PathMappings &Mappings) {
+  if (!S.startswith("file://"))
+    return llvm::None;
----------------
nit: add a comment like "bail out quickly in the common case"? to make it clear this is (only) a performance optimization


================
Comment at: clang-tools-extra/clangd/PathMapping.cpp:205
+  if (!Uri) {
+    return llvm::None;
+  }
----------------
you need to consume the error, or it'll assert

consumeError(Uri.takeError());


================
Comment at: clang-tools-extra/clangd/PathMapping.cpp:214
+      llvm::SmallString<256> MappedPath(Uri->body());
+      llvm::sys::path::replace_path_prefix(MappedPath, From, To);
+      auto MappedUri = URI(Uri->scheme(), Uri->authority(), MappedPath.c_str());
----------------
Sorry, I know I suggested replace_path_prefix, but now that the mappings consist of paths in their URL form, I think the old string::replace version is what you want :-/


================
Comment at: clang-tools-extra/clangd/PathMapping.h:26
+/// dependencies at different locations than the server.
 ///
 /// For example, if the mappings were {{"/home/user", "/workarea"}}, then
----------------
add a comment: `Therefore, both paths are represented as in file URI bodies, e.g. /etc/passwd or /C:/config.sys`


================
Comment at: clang-tools-extra/clangd/PathMapping.h:33
+  PathMapping() {}
+  PathMapping(llvm::StringRef ClientPath, llvm::StringRef ServerPath)
+      : ClientPath(ClientPath), ServerPath(ServerPath) {}
----------------
nit: you can probably drop these constructors and use `{}` aggregate initialization, up to you.


================
Comment at: clang-tools-extra/clangd/PathMapping.h:42
+
+/// Parse the command line \pRawPathMappings (e.g. "/client=/server") into
 /// pairs. Returns an error if the mappings are malformed, i.e. not absolute or
----------------
nit: please `\p RawPathMappings` with a space, or drop the doxygen tag entirely. These are read more often in the code than in rendered documentation, and `\pFoo` is hard to read.


================
Comment at: clang-tools-extra/clangd/PathMapping.h:56
 /// untouched.
-llvm::json::Value doPathMapping(const llvm::json::Value &Params,
-                                bool IsIncoming, const PathMappings &Mappings);
+void applyPathMappings(llvm::json::Value &Params, bool IsIncoming,
+                       const PathMappings &Mappings);
----------------
nit: the sense of the bool is pretty arbitrary here, prefer a two value enum?

e.g. `enum class PathMapping::Direction { ServerToClient, ClientToServer }`

(Reusing the "server" and "client" concept rather than adding "incoming/outgoing" seems a little simpler, though up to you)


================
Comment at: clang-tools-extra/clangd/unittests/PathMappingTests.cpp:131
+  EXPECT_TRUE(mapsProperly("file:///C:/home/foo.cpp",
+                           "file:///C:/workarea/foo.cpp", "C:/home=C:/workarea",
+                           true));
----------------
nit: please write the mapping as C:\ - it's probably redundant with the tests above, but it's clearer that it's on purpose


================
Comment at: clang-tools-extra/clangd/unittests/PathMappingTests.cpp:136
+TEST(DoPathMappingTests, MapsWindowsUnixInterop) {
+  // Path mappings with a windows-style client path and unix-style server path
+  EXPECT_TRUE(mapsProperly("file:///C:/home/foo.cpp",
----------------
The server path here isn't unix-style - that would be /foo/ or maybe C:/foo (windows path with unix-style slashes).

You could call this URI style, but I don't think this is something we'd want to (deliberately) support and we shouldn't have tests for it.


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

https://reviews.llvm.org/D64305