mstorsjo updated this revision to Diff 388857. mstorsjo added a comment. Added a comment about the surprising change where the order of append and native is switched. Made the modification to `testRoot()` less hacky.
Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113268/new/ https://reviews.llvm.org/D113268 Files: clang-tools-extra/clangd/tool/ClangdMain.cpp clang-tools-extra/clangd/unittests/TestFS.cpp clang-tools-extra/clangd/unittests/TestFS.h clang-tools-extra/clangd/unittests/URITests.cpp Index: clang-tools-extra/clangd/unittests/URITests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/URITests.cpp +++ clang-tools-extra/clangd/unittests/URITests.cpp @@ -133,8 +133,10 @@ TEST(URITest, Resolve) { #ifdef _WIN32 - EXPECT_THAT(resolveOrDie(parseOrDie("file:///c%3a/x/y/z")), "c:\\x\\y\\z"); - EXPECT_THAT(resolveOrDie(parseOrDie("file:///c:/x/y/z")), "c:\\x\\y\\z"); + llvm::SmallString<16> Ref("c:\\x\\y\\z"); + llvm::sys::path::native(Ref); + EXPECT_THAT(resolveOrDie(parseOrDie("file:///c%3a/x/y/z")), Ref); + EXPECT_THAT(resolveOrDie(parseOrDie("file:///c:/x/y/z")), Ref); #else EXPECT_EQ(resolveOrDie(parseOrDie("file:/a/b/c")), "/a/b/c"); EXPECT_EQ(resolveOrDie(parseOrDie("file://auth/a/b/c")), "//auth/a/b/c"); @@ -148,13 +150,17 @@ TEST(URITest, ResolveUNC) { #ifdef _WIN32 + llvm::SmallString<32> RefExample("\\\\example.com\\x\\y\\z"); + llvm::sys::path::native(RefExample); + llvm::SmallString<16> RefLocalhost("\\\\127.0.0.1\\x\\y\\z"); + llvm::sys::path::native(RefLocalhost); EXPECT_THAT(resolveOrDie(parseOrDie("file://example.com/x/y/z")), - "\\\\example.com\\x\\y\\z"); + RefExample); EXPECT_THAT(resolveOrDie(parseOrDie("file://127.0.0.1/x/y/z")), - "\\\\127.0.0.1\\x\\y\\z"); + RefLocalhost); // Ensure non-traditional file URI still resolves to correct UNC path. EXPECT_THAT(resolveOrDie(parseOrDie("file:////127.0.0.1/x/y/z")), - "\\\\127.0.0.1\\x\\y\\z"); + RefLocalhost); #else EXPECT_THAT(resolveOrDie(parseOrDie("file://example.com/x/y/z")), "//example.com/x/y/z"); Index: clang-tools-extra/clangd/unittests/TestFS.h =================================================================== --- clang-tools-extra/clangd/unittests/TestFS.h +++ clang-tools-extra/clangd/unittests/TestFS.h @@ -75,7 +75,7 @@ }; // Returns an absolute (fake) test directory for this OS. -const char *testRoot(); +std::string testRoot(); // Returns a suitable absolute path for this OS. std::string testPath(PathRef File, Index: clang-tools-extra/clangd/unittests/TestFS.cpp =================================================================== --- clang-tools-extra/clangd/unittests/TestFS.cpp +++ clang-tools-extra/clangd/unittests/TestFS.cpp @@ -71,9 +71,11 @@ FileName, std::move(CommandLine), "")}; } -const char *testRoot() { +std::string testRoot() { #ifdef _WIN32 - return "C:\\clangd-test"; + llvm::SmallString<16> NativeRoot("C:\\clangd-test"); + llvm::sys::path::native(NativeRoot); + return std::string(NativeRoot); #else return "/clangd-test"; #endif Index: clang-tools-extra/clangd/tool/ClangdMain.cpp =================================================================== --- clang-tools-extra/clangd/tool/ClangdMain.cpp +++ clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -541,15 +541,20 @@ Body); Body = Body.ltrim('/'); llvm::SmallString<16> Path(Body); - path::native(Path); fs::make_absolute(TestScheme::TestDir, Path); + // Convert the whole path to native separators after concatenating with + // TestDir; TestDir might contain separators other than the preferred + // on Windows if forward slashes are preferred. + path::native(Path); return std::string(Path); } llvm::Expected<URI> uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override { llvm::StringRef Body = AbsolutePath; - if (!Body.consume_front(TestScheme::TestDir)) + llvm::SmallString<16> NativeTestDir(TestDir); + llvm::sys::path::native(NativeTestDir); + if (!Body.consume_front(NativeTestDir)) return error("Path {0} doesn't start with root {1}", AbsolutePath, TestDir);
Index: clang-tools-extra/clangd/unittests/URITests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/URITests.cpp +++ clang-tools-extra/clangd/unittests/URITests.cpp @@ -133,8 +133,10 @@ TEST(URITest, Resolve) { #ifdef _WIN32 - EXPECT_THAT(resolveOrDie(parseOrDie("file:///c%3a/x/y/z")), "c:\\x\\y\\z"); - EXPECT_THAT(resolveOrDie(parseOrDie("file:///c:/x/y/z")), "c:\\x\\y\\z"); + llvm::SmallString<16> Ref("c:\\x\\y\\z"); + llvm::sys::path::native(Ref); + EXPECT_THAT(resolveOrDie(parseOrDie("file:///c%3a/x/y/z")), Ref); + EXPECT_THAT(resolveOrDie(parseOrDie("file:///c:/x/y/z")), Ref); #else EXPECT_EQ(resolveOrDie(parseOrDie("file:/a/b/c")), "/a/b/c"); EXPECT_EQ(resolveOrDie(parseOrDie("file://auth/a/b/c")), "//auth/a/b/c"); @@ -148,13 +150,17 @@ TEST(URITest, ResolveUNC) { #ifdef _WIN32 + llvm::SmallString<32> RefExample("\\\\example.com\\x\\y\\z"); + llvm::sys::path::native(RefExample); + llvm::SmallString<16> RefLocalhost("\\\\127.0.0.1\\x\\y\\z"); + llvm::sys::path::native(RefLocalhost); EXPECT_THAT(resolveOrDie(parseOrDie("file://example.com/x/y/z")), - "\\\\example.com\\x\\y\\z"); + RefExample); EXPECT_THAT(resolveOrDie(parseOrDie("file://127.0.0.1/x/y/z")), - "\\\\127.0.0.1\\x\\y\\z"); + RefLocalhost); // Ensure non-traditional file URI still resolves to correct UNC path. EXPECT_THAT(resolveOrDie(parseOrDie("file:////127.0.0.1/x/y/z")), - "\\\\127.0.0.1\\x\\y\\z"); + RefLocalhost); #else EXPECT_THAT(resolveOrDie(parseOrDie("file://example.com/x/y/z")), "//example.com/x/y/z"); Index: clang-tools-extra/clangd/unittests/TestFS.h =================================================================== --- clang-tools-extra/clangd/unittests/TestFS.h +++ clang-tools-extra/clangd/unittests/TestFS.h @@ -75,7 +75,7 @@ }; // Returns an absolute (fake) test directory for this OS. -const char *testRoot(); +std::string testRoot(); // Returns a suitable absolute path for this OS. std::string testPath(PathRef File, Index: clang-tools-extra/clangd/unittests/TestFS.cpp =================================================================== --- clang-tools-extra/clangd/unittests/TestFS.cpp +++ clang-tools-extra/clangd/unittests/TestFS.cpp @@ -71,9 +71,11 @@ FileName, std::move(CommandLine), "")}; } -const char *testRoot() { +std::string testRoot() { #ifdef _WIN32 - return "C:\\clangd-test"; + llvm::SmallString<16> NativeRoot("C:\\clangd-test"); + llvm::sys::path::native(NativeRoot); + return std::string(NativeRoot); #else return "/clangd-test"; #endif Index: clang-tools-extra/clangd/tool/ClangdMain.cpp =================================================================== --- clang-tools-extra/clangd/tool/ClangdMain.cpp +++ clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -541,15 +541,20 @@ Body); Body = Body.ltrim('/'); llvm::SmallString<16> Path(Body); - path::native(Path); fs::make_absolute(TestScheme::TestDir, Path); + // Convert the whole path to native separators after concatenating with + // TestDir; TestDir might contain separators other than the preferred + // on Windows if forward slashes are preferred. + path::native(Path); return std::string(Path); } llvm::Expected<URI> uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override { llvm::StringRef Body = AbsolutePath; - if (!Body.consume_front(TestScheme::TestDir)) + llvm::SmallString<16> NativeTestDir(TestDir); + llvm::sys::path::native(NativeTestDir); + if (!Body.consume_front(NativeTestDir)) return error("Path {0} doesn't start with root {1}", AbsolutePath, TestDir);
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits