sammccall added a comment. Thanks, this looks a lot better. Main thing that was unclear to me is the fact that the paths in the mappings are now URI-paths, so "/C:/foo" on windows.
This looks like the right idea as it ensures much of the path-mapping code gets to ignore slashes and such. Docs/naming could reflect it a bit better. ================ Comment at: clang-tools-extra/clangd/PathMapping.cpp:30 + const auto &K = V.kind(); + if (K == Kind::Object) { + for (auto &KV : *V.getAsObject()) { ---------------- wwagner19 wrote: > 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. Sorry, you're right - I'll fix `json::Object`. Nevertheless I think the copy-into-new-object approach is the clearest way to deal with renames of keys that may otherwise collide in the intermediate state. ================ Comment at: clang-tools-extra/clangd/PathMapping.cpp:36 using Kind = llvm::json::Value::Kind; - const auto &K = V.kind(); + const Kind &K = V.kind(); if (K == Kind::Object) { ---------------- by value, not by ref ================ Comment at: clang-tools-extra/clangd/PathMapping.cpp:137 +// Returns whether a path mapping should take place for \pOrigPath +// i.e. \pMappingPath is a proper sub-path of \p OrigPath ---------------- this isn't a doxygen comment, please omit \p ================ 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) { ---------------- "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? ================ Comment at: clang-tools-extra/clangd/PathMapping.cpp:201 + const PathMappings &Mappings) { + if (!S.startswith("file://")) + return llvm::None; ---------------- nit: add a comment like "bail out quickly in the common case"? to make it clear this is (only) a performance optimization ================ Comment at: clang-tools-extra/clangd/PathMapping.cpp:205 + if (!Uri) { + return llvm::None; + } ---------------- you need to consume the error, or it'll assert consumeError(Uri.takeError()); ================ 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()); ---------------- 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 :-/ ================ Comment at: clang-tools-extra/clangd/PathMapping.h:26 +/// dependencies at different locations than the server. /// /// For example, if the mappings were {{"/home/user", "/workarea"}}, then ---------------- add a comment: `Therefore, both paths are represented as in file URI bodies, e.g. /etc/passwd or /C:/config.sys` ================ Comment at: clang-tools-extra/clangd/PathMapping.h:33 + PathMapping() {} + PathMapping(llvm::StringRef ClientPath, llvm::StringRef ServerPath) + : ClientPath(ClientPath), ServerPath(ServerPath) {} ---------------- nit: you can probably drop these constructors and use `{}` aggregate initialization, up to you. ================ Comment at: clang-tools-extra/clangd/PathMapping.h:42 + +/// Parse the command line \pRawPathMappings (e.g. "/client=/server") into /// pairs. Returns an error if the mappings are malformed, i.e. not absolute or ---------------- nit: please `\p RawPathMappings` with a space, or drop the doxygen tag entirely. These are read more often in the code than in rendered documentation, and `\pFoo` is hard to read. ================ 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); ---------------- 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) ================ Comment at: clang-tools-extra/clangd/unittests/PathMappingTests.cpp:131 + EXPECT_TRUE(mapsProperly("file:///C:/home/foo.cpp", + "file:///C:/workarea/foo.cpp", "C:/home=C:/workarea", + true)); ---------------- nit: please write the mapping as C:\ - it's probably redundant with the tests above, but it's clearer that it's on purpose ================ Comment at: clang-tools-extra/clangd/unittests/PathMappingTests.cpp:136 +TEST(DoPathMappingTests, MapsWindowsUnixInterop) { + // Path mappings with a windows-style client path and unix-style server path + EXPECT_TRUE(mapsProperly("file:///C:/home/foo.cpp", ---------------- The server path here isn't unix-style - that would be /foo/ or maybe C:/foo (windows path with unix-style slashes). You could call this URI style, but I don't think this is something we'd want to (deliberately) support and we shouldn't have tests for it. 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