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