Skip to content

Commit 00b2089

Browse files
artemcmjansvoboda11
authored andcommitted
[clang][deps] Overload Filesystem::exists in DependencyScanningFilesystem to have it use cached status (llvm#88152)
As-is, calls to `exists()` fallback on the implementation in `ProxyFileSystem::exists` which explicitly calls out to the underlying `FS`, which for the `DependencyScanningFilesystem` (overlay) is the real underlying filesystem. Instead, directly overloading `exists` allows us to have it rely on the cached `status` behavior used elsewhere by the `DependencyScanningFilesystem`. (cherry picked from commit 779ba60)
1 parent 7d077a8 commit 00b2089

File tree

3 files changed

+42
-0
lines changed

3 files changed

+42
-0
lines changed

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,10 @@ class DependencyScanningWorkerFilesystem
381381
/// false if not (i.e. this entry is not a file or its scan fails).
382382
bool ensureDirectiveTokensArePopulated(EntryRef Entry);
383383

384+
/// Check whether \p Path exists. By default checks cached result of \c
385+
/// status(), and falls back on FS if unable to do so.
386+
bool exists(const Twine &Path) override;
387+
384388
private:
385389
/// For a filename that's not yet associated with any entry in the caches,
386390
/// uses the underlying filesystem to either look up the entry based in the

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,17 @@ DependencyScanningWorkerFilesystem::status(const Twine &Path) {
306306
return Result->getStatus();
307307
}
308308

309+
bool DependencyScanningWorkerFilesystem::exists(const Twine &Path) {
310+
// While some VFS overlay filesystems may implement more-efficient
311+
// mechanisms for `exists` queries, `DependencyScanningWorkerFilesystem`
312+
// typically wraps `RealFileSystem` which does not specialize `exists`,
313+
// so it is not likely to benefit from such optimizations. Instead,
314+
// it is more-valuable to have this query go through the
315+
// cached-`status` code-path of the `DependencyScanningWorkerFilesystem`.
316+
llvm::ErrorOr<llvm::vfs::Status> Status = status(Path);
317+
return Status && Status->exists();
318+
}
319+
309320
namespace {
310321

311322
/// The VFS that is used by clang consumes the \c CachedFileSystemEntry using

clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ struct InstrumentingFilesystem
1818
: llvm::RTTIExtends<InstrumentingFilesystem, llvm::vfs::ProxyFileSystem> {
1919
unsigned NumStatusCalls = 0;
2020
unsigned NumGetRealPathCalls = 0;
21+
unsigned NumExistsCalls = 0;
2122

2223
using llvm::RTTIExtends<InstrumentingFilesystem,
2324
llvm::vfs::ProxyFileSystem>::RTTIExtends;
@@ -32,6 +33,11 @@ struct InstrumentingFilesystem
3233
++NumGetRealPathCalls;
3334
return ProxyFileSystem::getRealPath(Path, Output);
3435
}
36+
37+
bool exists(const llvm::Twine &Path) override {
38+
++NumExistsCalls;
39+
return ProxyFileSystem::exists(Path);
40+
}
3541
};
3642
} // namespace
3743

@@ -147,3 +153,24 @@ TEST(DependencyScanningFilesystem, RealPathAndStatusInvariants) {
147153
DepFS.status("/bar");
148154
}
149155
}
156+
157+
TEST(DependencyScanningFilesystem, CacheStatOnExists) {
158+
auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
159+
auto InstrumentingFS =
160+
llvm::makeIntrusiveRefCnt<InstrumentingFilesystem>(InMemoryFS);
161+
InMemoryFS->setCurrentWorkingDirectory("/");
162+
InMemoryFS->addFile("/foo", 0, llvm::MemoryBuffer::getMemBuffer(""));
163+
InMemoryFS->addFile("/bar", 0, llvm::MemoryBuffer::getMemBuffer(""));
164+
DependencyScanningFilesystemSharedCache SharedCache;
165+
DependencyScanningWorkerFilesystem DepFS(SharedCache, InstrumentingFS);
166+
167+
DepFS.status("/foo");
168+
DepFS.status("/foo");
169+
DepFS.status("/bar");
170+
EXPECT_EQ(InstrumentingFS->NumStatusCalls, 2u);
171+
172+
DepFS.exists("/foo");
173+
DepFS.exists("/bar");
174+
EXPECT_EQ(InstrumentingFS->NumStatusCalls, 2u);
175+
EXPECT_EQ(InstrumentingFS->NumExistsCalls, 0u);
176+
}

0 commit comments

Comments
 (0)