[llvm] 35ab2a1 - Fix a buglet in remove_dots().
Reid Kleckner via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 2 11:07:52 PDT 2022
Author: Paul Pluzhnikov
Date: 2022-06-02T11:07:44-07:00
New Revision: 35ab2a11bb55c39ef9fe389aeacc14bb55c5a12d
URL: https://github.com/llvm/llvm-project/commit/35ab2a11bb55c39ef9fe389aeacc14bb55c5a12d
DIFF: https://github.com/llvm/llvm-project/commit/35ab2a11bb55c39ef9fe389aeacc14bb55c5a12d.diff
LOG: Fix a buglet in remove_dots().
The function promises to canonicalize the path, but neglected to do so
for the root component.
For example, calling remove_dots("/tmp/foo.c", Style::windows_backslash)
resulted in "/tmp\foo.c". Now it produces "\tmp\foo.c".
Also fix FIXME in the corresponding test.
Reviewed By: rnk
Differential Revision: https://reviews.llvm.org/D126412
Added:
Modified:
clang/unittests/Basic/FileManagerTest.cpp
clang/unittests/Frontend/PCHPreambleTest.cpp
llvm/lib/Support/Path.cpp
llvm/unittests/Support/Path.cpp
llvm/unittests/Support/VirtualFileSystemTest.cpp
Removed:
################################################################################
diff --git a/clang/unittests/Basic/FileManagerTest.cpp b/clang/unittests/Basic/FileManagerTest.cpp
index b8b12a91483d5..535d4d9e09c67 100644
--- a/clang/unittests/Basic/FileManagerTest.cpp
+++ b/clang/unittests/Basic/FileManagerTest.cpp
@@ -411,7 +411,7 @@ TEST_F(FileManagerTest, getVirtualFileWithDifferentName) {
#endif // !_WIN32
static StringRef getSystemRoot() {
- return is_style_windows(llvm::sys::path::Style::native) ? "C:/" : "/";
+ return is_style_windows(llvm::sys::path::Style::native) ? "C:\\" : "/";
}
TEST_F(FileManagerTest, makeAbsoluteUsesVFS) {
diff --git a/clang/unittests/Frontend/PCHPreambleTest.cpp b/clang/unittests/Frontend/PCHPreambleTest.cpp
index d253d937ace97..2ce24c91ac0f1 100644
--- a/clang/unittests/Frontend/PCHPreambleTest.cpp
+++ b/clang/unittests/Frontend/PCHPreambleTest.cpp
@@ -24,6 +24,13 @@ using namespace clang;
namespace {
+std::string Canonicalize(const Twine &Path) {
+ SmallVector<char, 128> PathVec;
+ Path.toVector(PathVec);
+ llvm::sys::path::remove_dots(PathVec, true);
+ return std::string(PathVec.begin(), PathVec.end());
+}
+
class ReadCountingInMemoryFileSystem : public vfs::InMemoryFileSystem
{
std::map<std::string, unsigned> ReadCounts;
@@ -31,16 +38,13 @@ class ReadCountingInMemoryFileSystem : public vfs::InMemoryFileSystem
public:
ErrorOr<std::unique_ptr<vfs::File>> openFileForRead(const Twine &Path) override
{
- SmallVector<char, 128> PathVec;
- Path.toVector(PathVec);
- llvm::sys::path::remove_dots(PathVec, true);
- ++ReadCounts[std::string(PathVec.begin(), PathVec.end())];
+ ++ReadCounts[Canonicalize(Path)];
return InMemoryFileSystem::openFileForRead(Path);
}
unsigned GetReadCount(const Twine &Path) const
{
- auto it = ReadCounts.find(Path.str());
+ auto it = ReadCounts.find(Canonicalize(Path));
return it == ReadCounts.end() ? 0 : it->second;
}
};
diff --git a/llvm/lib/Support/Path.cpp b/llvm/lib/Support/Path.cpp
index 973b903b48e61..9575e34b81c63 100644
--- a/llvm/lib/Support/Path.cpp
+++ b/llvm/lib/Support/Path.cpp
@@ -760,11 +760,15 @@ bool remove_dots(SmallVectorImpl<char> &the_path, bool remove_dot_dot,
}
}
+ SmallString<256> buffer = root;
+ // "root" could be "/", which may need to be translated into "\".
+ make_preferred(buffer, style);
+ needs_change |= root != buffer;
+
// Avoid rewriting the path unless we have to.
if (!needs_change)
return false;
- SmallString<256> buffer = root;
if (!components.empty()) {
buffer += components[0];
for (StringRef C : makeArrayRef(components).drop_front()) {
diff --git a/llvm/unittests/Support/Path.cpp b/llvm/unittests/Support/Path.cpp
index f609af6a0f96c..6f002276509a8 100644
--- a/llvm/unittests/Support/Path.cpp
+++ b/llvm/unittests/Support/Path.cpp
@@ -1544,16 +1544,14 @@ TEST(Support, RemoveDots) {
EXPECT_EQ("C:\\a\\c", remove_dots("C:\\foo\\bar//..\\..\\a\\c", true,
path::Style::windows));
- // FIXME: These leading forward slashes are emergent behavior. VFS depends on
- // this behavior now.
- EXPECT_EQ("C:/bar",
+ EXPECT_EQ("C:\\bar",
remove_dots("C:/foo/../bar", true, path::Style::windows));
- EXPECT_EQ("C:/foo\\bar",
+ EXPECT_EQ("C:\\foo\\bar",
remove_dots("C:/foo/bar", true, path::Style::windows));
- EXPECT_EQ("C:/foo\\bar",
+ EXPECT_EQ("C:\\foo\\bar",
remove_dots("C:/foo\\bar", true, path::Style::windows));
- EXPECT_EQ("/", remove_dots("/", true, path::Style::windows));
- EXPECT_EQ("C:/", remove_dots("C:/", true, path::Style::windows));
+ EXPECT_EQ("\\", remove_dots("/", true, path::Style::windows));
+ EXPECT_EQ("C:\\", remove_dots("C:/", true, path::Style::windows));
// Some clients of remove_dots expect it to remove trailing slashes. Again,
// this is emergent behavior that VFS relies on, and not inherently part of
@@ -1564,7 +1562,8 @@ TEST(Support, RemoveDots) {
remove_dots("/foo/bar/", true, path::Style::posix));
// A double separator is rewritten.
- EXPECT_EQ("C:/foo\\bar", remove_dots("C:/foo//bar", true, path::Style::windows));
+ EXPECT_EQ("C:\\foo\\bar",
+ remove_dots("C:/foo//bar", true, path::Style::windows));
SmallString<64> Path1(".\\.\\c");
EXPECT_TRUE(path::remove_dots(Path1, true, path::Style::windows));
diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp b/llvm/unittests/Support/VirtualFileSystemTest.cpp
index 52eba15e7a4b1..e32b3d2ef108e 100644
--- a/llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -199,7 +199,7 @@ class ErrorDummyFileSystem : public DummyFileSystem {
};
/// Replace back-slashes by front-slashes.
-std::string getPosixPath(std::string S) {
+std::string getPosixPath(const Twine &S) {
SmallString<128> Result;
llvm::sys::path::native(S, Result, llvm::sys::path::Style::posix);
return std::string(Result.str());
@@ -923,11 +923,11 @@ TEST(ProxyFileSystemTest, Basic) {
auto PWD = PFS.getCurrentWorkingDirectory();
ASSERT_FALSE(PWD.getError());
- ASSERT_EQ("/", *PWD);
+ ASSERT_EQ("/", getPosixPath(*PWD));
SmallString<16> Path;
ASSERT_FALSE(PFS.getRealPath("a", Path));
- ASSERT_EQ("/a", Path);
+ ASSERT_EQ("/a", getPosixPath(Path));
bool Local = true;
ASSERT_FALSE(PFS.isLocal("/a", Local));
@@ -1343,7 +1343,8 @@ TEST_F(InMemoryFileSystemTest, UniqueID) {
EXPECT_NE(FS.status("/a")->getUniqueID(), FS.status("/e")->getUniqueID());
// Recreating the "same" FS yields the same UniqueIDs.
- vfs::InMemoryFileSystem FS2;
+ // Note: FS2 should match FS with respect to path normalization.
+ vfs::InMemoryFileSystem FS2(/*UseNormalizedPath=*/false);
ASSERT_TRUE(FS2.addFile("/a/b", 0, MemoryBuffer::getMemBuffer("text")));
EXPECT_EQ(FS.status("/a/b")->getUniqueID(),
FS2.status("/a/b")->getUniqueID());
More information about the llvm-commits
mailing list