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

[William Wagner via Phabricator via cfe-commits]

wwagner19 marked 9 inline comments as done and an inline comment as not done.
wwagner19 added inline comments.


================
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) {
----------------
sammccall wrote:
> "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?
Oh I did not realize `createFile` was a thing, however looking at the way it's implemented now, won't that throw an assert if a non-absolute path is passed in? If so, is that desirable at all?

IIUC, if I were to use that API, then wouldn't it make more sense for it to return an `llvm::Expected`? If we want to consolidate the logic to one place, I'd be happy to try and refactor the signature.


================
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());
----------------
sammccall wrote:
> 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 :-/
Will do, I like string replace better anyway! 

I'm not a huge fan of the `mappingMatches` function, and would prefer a simple string `startswith(from)`, but the only way I may see that working is by appending "/" to all the path mappings internally - which would prevent `/foo` matching `/foo-bar` - but appending a "/" would break directory-based file URIs, I believe.


================
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);
----------------
sammccall wrote:
> 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)
much much better that way, thanks


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

https://reviews.llvm.org/D64305