sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay,
ilya-biryukov.
Herald added a project: clang.
- when we hit the cache, the reported filename should be that of the cache
query, not that of the cache store. This matches behaviors of common FSes, and
avoids triggering difficult edge cases in FileManager when files are being
moved around concurrently.
- filename comparisons (both cache queries and == mainfile checks) should fold
away . and .. in paths. These can appear when relative paths occur in
compile_commands.json. (gn does this).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D63931
Files:
clangd/FS.cpp
clangd/unittests/FSTests.cpp
Index: clangd/unittests/FSTests.cpp
===================================================================
--- clangd/unittests/FSTests.cpp
+++ clangd/unittests/FSTests.cpp
@@ -34,7 +34,7 @@
// Main file is not cached.
EXPECT_FALSE(StatCache.lookup(testPath("main")).hasValue());
- llvm::vfs::Status S("fake", llvm::sys::fs::UniqueID(0, 0),
+ llvm::vfs::Status S("fake", llvm::sys::fs::UniqueID(123, 456),
std::chrono::system_clock::now(), 0, 0, 1024,
llvm::sys::fs::file_type::regular_file,
llvm::sys::fs::all_all);
@@ -42,7 +42,15 @@
auto ConsumeFS = StatCache.getConsumingFS(FS);
auto Cached = ConsumeFS->status(testPath("fake"));
EXPECT_TRUE(Cached);
- EXPECT_EQ(Cached->getName(), S.getName());
+ EXPECT_EQ(Cached->getName(), testPath("fake"));
+ EXPECT_EQ(Cached->getUniqueID(), S.getUniqueID());
+
+ // fake and temp/../fake should hit the same cache entry.
+ // However, the Status returned reflects the actual path requested.
+ auto CachedDotDot = ConsumeFS->status(testPath("temp/../fake"));
+ EXPECT_TRUE(CachedDotDot);
+ EXPECT_EQ(CachedDotDot->getName(), testPath("temp/../fake"));
+ EXPECT_EQ(CachedDotDot->getUniqueID(), S.getUniqueID());
}
} // namespace
Index: clangd/FS.cpp
===================================================================
--- clangd/FS.cpp
+++ clangd/FS.cpp
@@ -10,20 +10,25 @@
#include "clang/Basic/LLVM.h"
#include "llvm/ADT/None.h"
#include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
namespace clang {
namespace clangd {
-PreambleFileStatusCache::PreambleFileStatusCache(llvm::StringRef MainFilePath)
- : MainFilePath(MainFilePath) {
+PreambleFileStatusCache::PreambleFileStatusCache(llvm::StringRef MainFilePath){
assert(llvm::sys::path::is_absolute(MainFilePath));
+ llvm::SmallString<256> MainFileCanonical(MainFilePath);
+ llvm::sys::path::remove_dots(MainFileCanonical, /*remove_dot_dot=*/true);
+ this->MainFilePath = MainFileCanonical.str();
}
void PreambleFileStatusCache::update(const llvm::vfs::FileSystem &FS,
llvm::vfs::Status S) {
+ // Canonicalize path for later lookup, which is usually by absolute path.
llvm::SmallString<32> PathStore(S.getName());
if (FS.makeAbsolute(PathStore))
return;
+ llvm::sys::path::remove_dots(PathStore, /*remove_dot_dot=*/true);
// Do not cache status for the main file.
if (PathStore == MainFilePath)
return;
@@ -33,9 +38,15 @@
llvm::Optional<llvm::vfs::Status>
PreambleFileStatusCache::lookup(llvm::StringRef File) const {
- auto I = StatCache.find(File);
+ // Canonicalize to match the cached form.
+ // Lookup tends to be first by absolute path, so no need to make absolute.
+ llvm::SmallString<256> PathLookup(File);
+ llvm::sys::path::remove_dots(PathLookup, /*remove_dot_dot=*/true);
+
+ auto I = StatCache.find(PathLookup);
if (I != StatCache.end())
- return I->getValue();
+ // Returned Status name should always match the requested File.
+ return llvm::vfs::Status::copyWithNewName(I->getValue(), File);
return None;
}
Index: clangd/unittests/FSTests.cpp
===================================================================
--- clangd/unittests/FSTests.cpp
+++ clangd/unittests/FSTests.cpp
@@ -34,7 +34,7 @@
// Main file is not cached.
EXPECT_FALSE(StatCache.lookup(testPath("main")).hasValue());
- llvm::vfs::Status S("fake", llvm::sys::fs::UniqueID(0, 0),
+ llvm::vfs::Status S("fake", llvm::sys::fs::UniqueID(123, 456),
std::chrono::system_clock::now(), 0, 0, 1024,
llvm::sys::fs::file_type::regular_file,
llvm::sys::fs::all_all);
@@ -42,7 +42,15 @@
auto ConsumeFS = StatCache.getConsumingFS(FS);
auto Cached = ConsumeFS->status(testPath("fake"));
EXPECT_TRUE(Cached);
- EXPECT_EQ(Cached->getName(), S.getName());
+ EXPECT_EQ(Cached->getName(), testPath("fake"));
+ EXPECT_EQ(Cached->getUniqueID(), S.getUniqueID());
+
+ // fake and temp/../fake should hit the same cache entry.
+ // However, the Status returned reflects the actual path requested.
+ auto CachedDotDot = ConsumeFS->status(testPath("temp/../fake"));
+ EXPECT_TRUE(CachedDotDot);
+ EXPECT_EQ(CachedDotDot->getName(), testPath("temp/../fake"));
+ EXPECT_EQ(CachedDotDot->getUniqueID(), S.getUniqueID());
}
} // namespace
Index: clangd/FS.cpp
===================================================================
--- clangd/FS.cpp
+++ clangd/FS.cpp
@@ -10,20 +10,25 @@
#include "clang/Basic/LLVM.h"
#include "llvm/ADT/None.h"
#include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
namespace clang {
namespace clangd {
-PreambleFileStatusCache::PreambleFileStatusCache(llvm::StringRef MainFilePath)
- : MainFilePath(MainFilePath) {
+PreambleFileStatusCache::PreambleFileStatusCache(llvm::StringRef MainFilePath){
assert(llvm::sys::path::is_absolute(MainFilePath));
+ llvm::SmallString<256> MainFileCanonical(MainFilePath);
+ llvm::sys::path::remove_dots(MainFileCanonical, /*remove_dot_dot=*/true);
+ this->MainFilePath = MainFileCanonical.str();
}
void PreambleFileStatusCache::update(const llvm::vfs::FileSystem &FS,
llvm::vfs::Status S) {
+ // Canonicalize path for later lookup, which is usually by absolute path.
llvm::SmallString<32> PathStore(S.getName());
if (FS.makeAbsolute(PathStore))
return;
+ llvm::sys::path::remove_dots(PathStore, /*remove_dot_dot=*/true);
// Do not cache status for the main file.
if (PathStore == MainFilePath)
return;
@@ -33,9 +38,15 @@
llvm::Optional<llvm::vfs::Status>
PreambleFileStatusCache::lookup(llvm::StringRef File) const {
- auto I = StatCache.find(File);
+ // Canonicalize to match the cached form.
+ // Lookup tends to be first by absolute path, so no need to make absolute.
+ llvm::SmallString<256> PathLookup(File);
+ llvm::sys::path::remove_dots(PathLookup, /*remove_dot_dot=*/true);
+
+ auto I = StatCache.find(PathLookup);
if (I != StatCache.end())
- return I->getValue();
+ // Returned Status name should always match the requested File.
+ return llvm::vfs::Status::copyWithNewName(I->getValue(), File);
return None;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
