[clang-tools-extra] r364740 - [clangd] Make PreambleStatusCache handle filenames more carefully
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 1 03:11:18 PDT 2019
Author: sammccall
Date: Mon Jul 1 03:11:18 2019
New Revision: 364740
URL: http://llvm.org/viewvc/llvm-project?rev=364740&view=rev
Log:
[clangd] Make PreambleStatusCache handle filenames more carefully
Summary:
- 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).
Reviewers: kadircet
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D63931
Modified:
clang-tools-extra/trunk/clangd/FS.cpp
clang-tools-extra/trunk/clangd/unittests/FSTests.cpp
Modified: clang-tools-extra/trunk/clangd/FS.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/FS.cpp?rev=364740&r1=364739&r2=364740&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/FS.cpp (original)
+++ clang-tools-extra/trunk/clangd/FS.cpp Mon Jul 1 03:11:18 2019
@@ -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 @@ void PreambleFileStatusCache::update(con
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;
}
Modified: clang-tools-extra/trunk/clangd/unittests/FSTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/FSTests.cpp?rev=364740&r1=364739&r2=364740&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/unittests/FSTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/FSTests.cpp Mon Jul 1 03:11:18 2019
@@ -34,7 +34,7 @@ TEST(FSTests, PreambleStatusCache) {
// 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 @@ TEST(FSTests, PreambleStatusCache) {
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
More information about the cfe-commits
mailing list