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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
