wwagner19 updated this revision to Diff 229721.
wwagner19 marked 5 inline comments as done.
wwagner19 added a comment.
Thanks for the second review Sam, I addressed most of your comments, notably:
- Changed the bool IsIncoming to an enum
- Fixed the "doxygen" comments,
- Removed some redundant incudes/variables
- Switched `replace_path_prefix` to string replace
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64305/new/
https://reviews.llvm.org/D64305
Files:
clang-tools-extra/clangd/PathMapping.cpp
clang-tools-extra/clangd/PathMapping.h
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/PathMappingTests.cpp
Index: clang-tools-extra/clangd/unittests/PathMappingTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/PathMappingTests.cpp
+++ clang-tools-extra/clangd/unittests/PathMappingTests.cpp
@@ -82,12 +82,11 @@
}
bool mapsProperly(llvm::StringRef Orig, llvm::StringRef Expected,
- llvm::StringRef RawMappings, bool IsIncoming) {
+ llvm::StringRef RawMappings, PathMapping::Direction Dir) {
llvm::Expected<PathMappings> Mappings = parsePathMappings(RawMappings);
if (!Mappings)
return false;
- llvm::Optional<std::string> MappedPath =
- doPathMapping(Orig, IsIncoming, *Mappings);
+ llvm::Optional<std::string> MappedPath = doPathMapping(Orig, Dir, *Mappings);
std::string Actual = MappedPath ? *MappedPath : Orig.str();
EXPECT_STREQ(Expected.str().c_str(), Actual.c_str());
return Expected == Actual;
@@ -95,48 +94,54 @@
TEST(DoPathMappingTests, PreservesOriginal) {
// Preserves original path when no mapping
- EXPECT_TRUE(mapsProperly("file:///home", "file:///home", "", true));
+ EXPECT_TRUE(mapsProperly("file:///home", "file:///home", "",
+ PathMapping::Direction::ClientToServer));
}
TEST(DoPathMappingTests, UsesFirstMatch) {
EXPECT_TRUE(mapsProperly("file:///home/foo.cpp", "file:///workarea1/foo.cpp",
- "/home=/workarea1,/home=/workarea2", true));
+ "/home=/workarea1,/home=/workarea2",
+ PathMapping::Direction::ClientToServer));
}
TEST(DoPathMappingTests, IgnoresSubstrings) {
// Doesn't map substrings that aren't a proper path prefix
EXPECT_TRUE(mapsProperly("file://home/foo-bar.cpp", "file://home/foo-bar.cpp",
- "/home/foo=/home/bar", true));
+ "/home/foo=/home/bar",
+ PathMapping::Direction::ClientToServer));
}
TEST(DoPathMappingTests, MapsOutgoingPaths) {
// When IsIncoming is false (i.e.a response), map the other way
EXPECT_TRUE(mapsProperly("file:///workarea/foo.cpp", "file:///home/foo.cpp",
- "/home=/workarea", false));
+ "/home=/workarea",
+ PathMapping::Direction::ServerToClient));
}
TEST(DoPathMappingTests, OnlyMapFileUris) {
EXPECT_TRUE(mapsProperly("test:///home/foo.cpp", "test:///home/foo.cpp",
- "/home=/workarea", true));
+ "/home=/workarea",
+ PathMapping::Direction::ClientToServer));
}
TEST(DoPathMappingTests, RespectsCaseSensitivity) {
EXPECT_TRUE(mapsProperly("file:///HOME/foo.cpp", "file:///HOME/foo.cpp",
- "/home=/workarea", true));
+ "/home=/workarea",
+ PathMapping::Direction::ClientToServer));
}
TEST(DoPathMappingTests, MapsWindowsPaths) {
// Maps windows properly
EXPECT_TRUE(mapsProperly("file:///C:/home/foo.cpp",
- "file:///C:/workarea/foo.cpp", "C:/home=C:/workarea",
- true));
+ "file:///C:/workarea/foo.cpp", R"(C:\home=C:\workarea)",
+ PathMapping::Direction::ClientToServer));
}
TEST(DoPathMappingTests, MapsWindowsUnixInterop) {
// Path mappings with a windows-style client path and unix-style server path
- EXPECT_TRUE(mapsProperly("file:///C:/home/foo.cpp",
- "file:///C:/workarea/foo.cpp",
- "C:/home=/C:/workarea", true));
+ EXPECT_TRUE(mapsProperly(
+ "file:///C:/home/foo.cpp", "file:///workarea/foo.cpp",
+ R"(C:\home=/workarea)", PathMapping::Direction::ClientToServer));
}
TEST(ApplyPathMappingTests, PreservesOriginalParams) {
@@ -147,7 +152,7 @@
ASSERT_TRUE(bool(Params));
llvm::json::Value ExpectedParams = *Params;
PathMappings Mappings;
- applyPathMappings(*Params, /*IsIncoming=*/true, Mappings);
+ applyPathMappings(*Params, PathMapping::Direction::ClientToServer, Mappings);
EXPECT_EQ(*Params, ExpectedParams);
}
@@ -163,7 +168,7 @@
})");
auto Mappings = parsePathMappings("/home=/workarea");
ASSERT_TRUE(bool(Params) && bool(ExpectedParams) && bool(Mappings));
- applyPathMappings(*Params, /*IsIncoming=*/true, *Mappings);
+ applyPathMappings(*Params, PathMapping::Direction::ClientToServer, *Mappings);
EXPECT_EQ(*Params, *ExpectedParams);
}
@@ -183,7 +188,7 @@
auto Mappings =
parsePathMappings("/home=/workarea,/home/.includes=/opt/include");
ASSERT_TRUE(bool(Params) && bool(ExpectedParams) && bool(Mappings));
- applyPathMappings(*Params, /*IsIncoming=*/false, *Mappings);
+ applyPathMappings(*Params, PathMapping::Direction::ServerToClient, *Mappings);
EXPECT_EQ(*Params, *ExpectedParams);
}
@@ -202,7 +207,7 @@
})");
auto Mappings = parsePathMappings("/home=/workarea");
ASSERT_TRUE(bool(Params) && bool(ExpectedParams) && bool(Mappings));
- applyPathMappings(*Params, /*IsIncoming=*/true, *Mappings);
+ applyPathMappings(*Params, PathMapping::Direction::ClientToServer, *Mappings);
EXPECT_EQ(*Params, *ExpectedParams);
}
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===================================================================
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -346,7 +346,7 @@
"location where clangd is running,"
"e.g. "
"/home/project=/workarea/project,/home/project/.includes=/opt/include"
- "opt/include"),
+ ),
llvm::cl::init(""));
opt<Path> InputMirrorFile{
Index: clang-tools-extra/clangd/PathMapping.h
===================================================================
--- clang-tools-extra/clangd/PathMapping.h
+++ clang-tools-extra/clangd/PathMapping.h
@@ -11,7 +11,7 @@
#include "llvm/Support/JSON.h"
#include "llvm/Support/raw_ostream.h"
#include <memory>
-#include <tuple>
+#include <string>
#include <vector>
namespace clang {
@@ -22,43 +22,43 @@
/// PathMappings are a collection of paired client and server paths.
/// These pairs are used to alter file:// URIs appearing in inbound and outbound
/// LSP messages, as the client's environment may have source files or
-/// dependencies at different locations than the server.
+/// dependencies at different locations than the server. Therefore, both
+/// paths are stored as they appear in file URI bodies, e.g. /usr/include or
+/// /C:/config
///
/// For example, if the mappings were {{"/home/user", "/workarea"}}, then
-/// an inbound LSP message would have file:///home/user/foo.cpp remapped to
-/// file:///workarea/foo.cpp, and the same would happen for replies (in the
-/// opposite order).
+/// a client-to-server LSP message would have file:///home/user/foo.cpp
+/// remapped to file:///workarea/foo.cpp, and the same would happen for replies
+/// (in the opposite order).
struct PathMapping {
- PathMapping() {}
- PathMapping(llvm::StringRef ClientPath, llvm::StringRef ServerPath)
- : ClientPath(ClientPath), ServerPath(ServerPath) {}
std::string ClientPath;
std::string ServerPath;
+ enum class Direction { ClientToServer, ServerToClient };
};
using PathMappings = std::vector<PathMapping>;
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const PathMapping &M);
-/// Parse the command line \pRawPathMappings (e.g. "/client=/server") into
+/// Parse the command line \p RawPathMappings (e.g. "/client=/server") into
/// pairs. Returns an error if the mappings are malformed, i.e. not absolute or
/// not a proper pair.
llvm::Expected<PathMappings> parsePathMappings(llvm::StringRef RawPathMappings);
-/// Returns a modified \pS with the first matching path in \pPathMappings
+/// Returns a modified \p S with the first matching path in \p Mappings
/// substituted, if applicable
-llvm::Optional<std::string> doPathMapping(llvm::StringRef S, bool IsIncoming,
+llvm::Optional<std::string> doPathMapping(llvm::StringRef S,
+ PathMapping::Direction Dir,
const PathMappings &Mappings);
-/// Applies the \pMappings to all the file:// URIs in \pParams.
-/// \pIsIncoming affects which direction the mappings are applied.
-/// NOTE: The first matching mapping will be applied, otherwise \pParams will be
-/// untouched.
-void applyPathMappings(llvm::json::Value &Params, bool IsIncoming,
+/// Applies the \p Mappings to all the file:// URIs in \p Params.
+/// NOTE: The first matching mapping will be applied, otherwise \p Params will
+/// be untouched.
+void applyPathMappings(llvm::json::Value &Params, PathMapping::Direction Dir,
const PathMappings &Mappings);
-/// Creates a wrapping transport over \pTransp that applies the \pMappings to
+/// Creates a wrapping transport over \p Transp that applies the \p Mappings to
/// all inbound and outbound LSP messages. All calls are then delegated to the
-/// regular \pTransp (e.g. XPC, JSON).
+/// regular transport (e.g. XPC, JSON).
std::unique_ptr<Transport>
createPathMappingTransport(std::unique_ptr<Transport> Transp,
PathMappings Mappings);
Index: clang-tools-extra/clangd/PathMapping.cpp
===================================================================
--- clang-tools-extra/clangd/PathMapping.cpp
+++ clang-tools-extra/clangd/PathMapping.cpp
@@ -6,41 +6,33 @@
//
//===----------------------------------------------------------------------===//
#include "PathMapping.h"
-#include "Logger.h"
#include "Transport.h"
#include "URI.h"
#include "llvm/ADT/None.h"
-#include "llvm/ADT/Optional.h"
#include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/SmallString.h"
-#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Errno.h"
-#include "llvm/Support/JSON.h"
#include "llvm/Support/Path.h"
-#include "llvm/Support/raw_ostream.h"
#include <algorithm>
-#include <memory>
#include <tuple>
-#include <vector>
namespace clang {
namespace clangd {
namespace {
using MapperFunc = llvm::function_ref<llvm::Optional<std::string>(
- llvm::StringRef, bool, const PathMappings &)>;
+ llvm::StringRef, PathMapping::Direction, const PathMappings &)>;
-// Recursively apply the \pMF to all string keys/values in \pV
-void recursivelyMap(llvm::json::Value &V, bool IsIncoming,
+// Recursively apply the MF to all string keys/values in V
+void recursivelyMap(llvm::json::Value &V, PathMapping::Direction Dir,
const PathMappings &Mappings, const MapperFunc &MF) {
using Kind = llvm::json::Value::Kind;
- const Kind &K = V.kind();
+ Kind K = V.kind();
if (K == Kind::Object) {
llvm::json::Object *Obj = V.getAsObject();
llvm::json::Object MappedObj;
// 1. Map all the Keys
for (auto &KV : *Obj) {
if (llvm::Optional<std::string> MappedKey =
- MF(KV.first.str(), IsIncoming, Mappings)) {
+ MF(KV.first.str(), Dir, Mappings)) {
MappedObj.try_emplace(std::move(*MappedKey), std::move(KV.second));
} else {
MappedObj.try_emplace(std::move(KV.first), std::move(KV.second));
@@ -49,15 +41,15 @@
*Obj = std::move(MappedObj);
// 2. Map all the values
for (auto &KV : *Obj) {
- recursivelyMap(KV.second, IsIncoming, Mappings, MF);
+ recursivelyMap(KV.second, Dir, Mappings, MF);
}
} else if (K == Kind::Array) {
for (llvm::json::Value &Val : *V.getAsArray()) {
- recursivelyMap(Val, IsIncoming, Mappings, MF);
+ recursivelyMap(Val, Dir, Mappings, MF);
}
} else if (K == Kind::String) {
if (llvm::Optional<std::string> Mapped =
- MF(*V.getAsString(), IsIncoming, Mappings)) {
+ MF(*V.getAsString(), Dir, Mappings)) {
V = std::move(*Mapped);
}
}
@@ -70,20 +62,21 @@
: WrappedHandler(Handler), Mappings(Mappings) {}
bool onNotify(llvm::StringRef Method, llvm::json::Value Params) override {
- applyPathMappings(Params, /*IsIncoming=*/true, Mappings);
+ applyPathMappings(Params, PathMapping::Direction::ClientToServer, Mappings);
return WrappedHandler.onNotify(Method, std::move(Params));
}
bool onCall(llvm::StringRef Method, llvm::json::Value Params,
llvm::json::Value ID) override {
- applyPathMappings(Params, /*IsIncoming=*/true, Mappings);
+ applyPathMappings(Params, PathMapping::Direction::ClientToServer, Mappings);
return WrappedHandler.onCall(Method, std::move(Params), std::move(ID));
}
bool onReply(llvm::json::Value ID,
llvm::Expected<llvm::json::Value> Result) override {
if (Result) {
- applyPathMappings(*Result, /*IsIncoming=*/true, Mappings);
+ applyPathMappings(*Result, PathMapping::Direction::ClientToServer,
+ Mappings);
}
return WrappedHandler.onReply(std::move(ID), std::move(Result));
}
@@ -101,20 +94,21 @@
: WrappedTransport(std::move(Transp)), Mappings(std::move(Mappings)) {}
void notify(llvm::StringRef Method, llvm::json::Value Params) override {
- applyPathMappings(Params, /*IsIncoming=*/false, Mappings);
+ applyPathMappings(Params, PathMapping::Direction::ServerToClient, Mappings);
WrappedTransport->notify(Method, std::move(Params));
}
void call(llvm::StringRef Method, llvm::json::Value Params,
llvm::json::Value ID) override {
- applyPathMappings(Params, /*IsIncoming=*/false, Mappings);
+ applyPathMappings(Params, PathMapping::Direction::ServerToClient, Mappings);
WrappedTransport->call(Method, std::move(Params), std::move(ID));
}
void reply(llvm::json::Value ID,
llvm::Expected<llvm::json::Value> Result) override {
if (Result) {
- applyPathMappings(*Result, /*IsIncoming=*/false, Mappings);
+ applyPathMappings(*Result, PathMapping::Direction::ServerToClient,
+ Mappings);
}
WrappedTransport->reply(std::move(ID), std::move(Result));
}
@@ -134,8 +128,8 @@
llvm::inconvertibleErrorCode());
}
-// Returns whether a path mapping should take place for \pOrigPath
-// i.e. \pMappingPath is a proper sub-path of \p OrigPath
+// Returns whether a path mapping should take place for OrigPath
+// i.e. MappingPath is a proper sub-path of OrigPath
bool mappingMatches(llvm::StringRef OrigPath, llvm::StringRef MappingPath) {
namespace path = llvm::sys::path;
auto OrigPathItr = path::begin(OrigPath, path::Style::posix);
@@ -149,11 +143,10 @@
return std::equal(MappingPathItr, MappingPathEnd, OrigPathItr);
}
-// 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
+// Converts a unix/windows path to the path portion of a file URI
+// e.g. "C:\foo" -> "/C:/foo"
llvm::Expected<std::string> parsePath(llvm::StringRef Path) {
namespace path = llvm::sys::path;
- std::string ParsedPath;
if (path::is_absolute(Path, path::Style::posix)) {
return Path;
} else if (path::is_absolute(Path, path::Style::windows)) {
@@ -190,38 +183,43 @@
if (!ParsedServerPath) {
return ParsedServerPath.takeError();
}
- ParsedMappings.emplace_back(std::move(*ParsedClientPath),
- std::move(*ParsedServerPath));
+ ParsedMappings.push_back(
+ {std::move(*ParsedClientPath), std::move(*ParsedServerPath)});
}
return ParsedMappings;
}
-llvm::Optional<std::string> doPathMapping(llvm::StringRef S, bool IsIncoming,
+llvm::Optional<std::string> doPathMapping(llvm::StringRef S,
+ PathMapping::Direction Dir,
const PathMappings &Mappings) {
+ // Retrun early to optimize for the common case, wherein S is not a file URI
if (!S.startswith("file://"))
return llvm::None;
auto Uri = URI::parse(S);
if (!Uri) {
+ llvm::consumeError(Uri.takeError());
return llvm::None;
}
for (const auto &Mapping : Mappings) {
- const std::string &From =
- IsIncoming ? Mapping.ClientPath : Mapping.ServerPath;
- const std::string &To =
- IsIncoming ? Mapping.ServerPath : Mapping.ClientPath;
+ const std::string &From = Dir == PathMapping::Direction::ClientToServer
+ ? Mapping.ClientPath
+ : Mapping.ServerPath;
+ const std::string &To = Dir == PathMapping::Direction::ClientToServer
+ ? Mapping.ServerPath
+ : Mapping.ClientPath;
if (mappingMatches(Uri->body(), From)) {
- 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());
+ std::string MappedBody = std::move(Uri->body());
+ MappedBody.replace(MappedBody.find(From), From.length(), To);
+ auto MappedUri = URI(Uri->scheme(), Uri->authority(), MappedBody.c_str());
return MappedUri.toString();
}
}
return llvm::None;
}
-void applyPathMappings(llvm::json::Value &Params, bool IsIncoming,
+void applyPathMappings(llvm::json::Value &Params, PathMapping::Direction Dir,
const PathMappings &Mappings) {
- recursivelyMap(Params, IsIncoming, Mappings, doPathMapping);
+ recursivelyMap(Params, Dir, Mappings, doPathMapping);
}
std::unique_ptr<Transport>
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
