wwagner19 marked 9 inline comments as done.
wwagner19 added a comment.
Thanks for all the feedback, Sam! I'll try and get an updated patch up sometime
tomorrow.
================
Comment at: clang-tools-extra/clangd/PathMapping.cpp:30
+ const auto &K = V.kind();
+ if (K == Kind::Object) {
+ for (auto &KV : *V.getAsObject()) {
----------------
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.
================
Comment at: clang-tools-extra/clangd/PathMapping.cpp:51
+ llvm::json::Value MappedParams =
+ doPathMapping(Params, /*IsIncoming=*/true, Mappings);
+ return WrappedHandler.onNotify(Method, std::move(MappedParams));
----------------
sammccall wrote:
> not a really big deal, but we're forcing a copy here - should doPathMapping
> mutate its argument and return void instead?
yup makes sense!
================
Comment at: clang-tools-extra/clangd/PathMapping.cpp:165
+ const auto &To = IsIncoming ? Mapping.second : Mapping.first;
+ if (Uri->body().startswith(From)) {
+ std::string MappedBody = Uri->body();
----------------
sammccall wrote:
> This is simplistic I'm afraid: it's not going to work at all on windows
> (where paths have `\` but URIs have `/`), and it's going to falsely match
> "/foo-bar/baz" against a mapping for "/foo".
>
> `llvm::sys::fs::replace_path_prefix` is probably the beginning of a solution.
> If I'm reading correctly the first two args need to have the same slash style
> and OldPrefix should have its trailing slash.
>
> I'd actually suggest pulling out the function to map one string, and
> unit-testing that, to catch all the filename cases.
>
> Then the json::Value traversal tests should be focused on testing the places
> where a string can appear in JSON, not all the different contents of the
> string.
Ah yea good catch, this will be a bit more tricky then. I was originally just
imagining the users using strictly URI syntax in the `--path-mappings`, but
that's doesn't seem very friendly in hindsight. So just to clarify, we should:
1. Allow the users to specify windows style paths (e.g. C:\foo) and posix style
paths
2. Allow the inter-op of both, i.e. `--path-mappings="C:\foo=/bar"`
IIUC, file URIs will always have the forward-slash syntax, so this may require
storing the windows-style path mapping in forward-slash style. I can try and
get this going tomorrow. Although, one tricky thing might be trying to figure
out if a path is indeed windows-style (in a unix environment where _WIN32 isn't
defined).
================
Comment at: clang-tools-extra/clangd/PathMapping.h:21
+
+/// PathMappings are a collection of paired host and remote paths.
+/// These pairs are used to alter file:// URIs appearing in inbound and
outbound
----------------
sammccall wrote:
> hmm, the host/remote terminology is a bit confusing to me.
> I guess the host is the machine where clangd is running, and remote is the
> other end, but from the user's point of view this is a configuration where
> the clangd is remote.
>
> What about calling these client/server paths? The language client/server
> roles are defined in the spec and these terms are likely to make some sense
> to users.
Agreed, sounds better
================
Comment at: clang-tools-extra/clangd/PathMapping.h:32
+
+/// Parse the command line \pRawPathMappings (e.g. "/host|/remote") into
+/// pairs. Returns an error if the mappings are malformed, i.e. not absolute or
----------------
sammccall wrote:
> I'd suggest `=` as a delimiter instead, it's more evocative and more common.
>
> The order is tricky, I suspect `/client/path=/server/path` is going to be
> more intuitive
Way better :)
================
Comment at:
clang-tools-extra/clangd/test/Inputs/path-mappings/definition.jsonrpc:26
+---
+{
+ "jsonrpc": "2.0",
----------------
sammccall wrote:
> This test seems to have unneccesary moving parts. Was it copied from the
> background index test?
>
> One header file on disk and one draft file sent over RPC should be enough. A
> compilation database shouldn't be necessary I think.
>
> You shouldn't need to splice URIs, because the input and output paths are
> virtual and fully under your control (that's the point of this patch, :)). So
> the test should be able to run on windows.
>
> I think this test can actually be a standard one where the JSON-RPC calls and
> assertions go in the *.test file.
This was copied from the background test, I felt a bit uneasy about how
complicated it got, but I had a bit of trouble getting a simpler one going.
You're right though, I can't see why this wouldn't work with a "remote" file on
disk, and having the draft RPC file simply include that, i'll have another go
at it.
Repository:
rCTE Clang Tools Extra
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
