Skip to content

Commit 779ba60

Browse files
authored
[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`.
1 parent db8e182 commit 779ba60

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
@@ -365,6 +365,10 @@ class DependencyScanningWorkerFilesystem
365365
/// false if not (i.e. this entry is not a file or its scan fails).
366366
bool ensureDirectiveTokensArePopulated(EntryRef Entry);
367367

368+
/// Check whether \p Path exists. By default checks cached result of \c
369+
/// status(), and falls back on FS if unable to do so.
370+
bool exists(const Twine &Path) override;
371+
368372
private:
369373
/// For a filename that's not yet associated with any entry in the caches,
370374
/// 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
@@ -300,6 +300,17 @@ DependencyScanningWorkerFilesystem::status(const Twine &Path) {
300300
return Result->getStatus();
301301
}
302302

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

305316
/// 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)