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

[William Wagner via Phabricator via cfe-commits]

wwagner19 marked 2 inline comments as 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:
> wwagner19 wrote:
> > 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.
> I think allowing relative paths to be specified in path mappings is needlessly confusing. Clangd flags are typically specified in a config file somewhere, and that config often doesn't know about the working directory.
> 
> We don't want to hit the assert, so I'd suggest testing if it's absolute first, and returning an error if not.
So even with checking for absolute paths before calling `createFile`, it still won't work quite right. Currently, `createFile`, and consequently `FileSystemScheme().uriFromAbsolutePath(AbsolutePath)`, converts paths differently depending on the environment Clangd is running on (via WIN32 or some other means).

e.g. if we had mapping `C:\home=/workarea` and Clangd built/running on linux, then the `replace_path_prefix` by default would use posix style, which won't replace the `\`. This may not be **too** useful in practice, but it's a small price to pay for windows-unix-interop, I feel.


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

https://reviews.llvm.org/D64305