From b2a7f1c3904ede781565c6ed772f155167aa8325 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Wed, 6 Apr 2022 11:37:25 +0200 Subject: [PATCH] Remove a few effectively-unused FileEntry APIs. NFC - isValid: FileManager only ever returns valid FileEntries (see next point) - construction from outside FileManager (both FileEntry and DirectoryEntry). It's not possible to create a useful FileEntry this way, there are no setters. This was only used in FileEntryTest, added a friend to enable this. A real constructor is cleaner but requires larger changes to FileManager. Differential Revision: https://reviews.llvm.org/D123197 --- clang/include/clang/Basic/DirectoryEntry.h | 4 +++ clang/include/clang/Basic/FileEntry.h | 14 +++------ clang/lib/Basic/FileManager.cpp | 3 -- clang/lib/Frontend/LogDiagnosticPrinter.cpp | 6 ++-- clang/lib/Frontend/TextDiagnostic.cpp | 3 +- clang/unittests/Basic/FileEntryTest.cpp | 46 +++++++++++------------------ clang/unittests/Basic/FileManagerTest.cpp | 13 -------- 7 files changed, 28 insertions(+), 61 deletions(-) diff --git a/clang/include/clang/Basic/DirectoryEntry.h b/clang/include/clang/Basic/DirectoryEntry.h index 2156562..fe18b12 100644 --- a/clang/include/clang/Basic/DirectoryEntry.h +++ b/clang/include/clang/Basic/DirectoryEntry.h @@ -32,7 +32,11 @@ template class MapEntryOptionalStorage; /// Cached information about one directory (either on disk or in /// the virtual file system). class DirectoryEntry { + DirectoryEntry() = default; + DirectoryEntry(const DirectoryEntry &) = delete; + DirectoryEntry &operator=(const DirectoryEntry &) = delete; friend class FileManager; + friend class FileEntryTestHelper; // FIXME: We should not be storing a directory entry name here. StringRef Name; // Name of the directory. diff --git a/clang/include/clang/Basic/FileEntry.h b/clang/include/clang/Basic/FileEntry.h index a0a041c..92a6715 100644 --- a/clang/include/clang/Basic/FileEntry.h +++ b/clang/include/clang/Basic/FileEntry.h @@ -65,7 +65,6 @@ public: } DirectoryEntryRef getDir() const { return *ME->second->Dir; } - inline bool isValid() const; inline off_t getSize() const; inline unsigned getUID() const; inline const llvm::sys::fs::UniqueID &getUniqueID() const; @@ -330,6 +329,10 @@ static_assert( /// descriptor for the file. class FileEntry { friend class FileManager; + friend class FileEntryTestHelper; + FileEntry(); + FileEntry(const FileEntry &) = delete; + FileEntry &operator=(const FileEntry &) = delete; std::string RealPathName; // Real path to the file; could be empty. off_t Size = 0; // File size in bytes. @@ -338,7 +341,6 @@ class FileEntry { llvm::sys::fs::UniqueID UniqueID; unsigned UID = 0; // A unique (small) ID for the file. bool IsNamedPipe = false; - bool IsValid = false; // Is this \c FileEntry initialized and valid? /// The open file, if it is owned by the \p FileEntry. mutable std::unique_ptr File; @@ -355,17 +357,11 @@ class FileEntry { Optional LastRef; public: - FileEntry(); ~FileEntry(); - - FileEntry(const FileEntry &) = delete; - FileEntry &operator=(const FileEntry &) = delete; - StringRef getName() const { return LastRef->getName(); } FileEntryRef getLastRef() const { return *LastRef; } StringRef tryGetRealPathName() const { return RealPathName; } - bool isValid() const { return IsValid; } off_t getSize() const { return Size; } unsigned getUID() const { return UID; } const llvm::sys::fs::UniqueID &getUniqueID() const { return UniqueID; } @@ -381,8 +377,6 @@ public: void closeFile() const; }; -bool FileEntryRef::isValid() const { return getFileEntry().isValid(); } - off_t FileEntryRef::getSize() const { return getFileEntry().getSize(); } unsigned FileEntryRef::getUID() const { return getFileEntry().getUID(); } diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp index b955e02..baabb32 100644 --- a/clang/lib/Basic/FileManager.cpp +++ b/clang/lib/Basic/FileManager.cpp @@ -342,7 +342,6 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) { UFE->UniqueID = Status.getUniqueID(); UFE->IsNamedPipe = Status.getType() == llvm::sys::fs::file_type::fifo_file; UFE->File = std::move(F); - UFE->IsValid = true; if (UFE->File) { if (auto PathName = UFE->File->getName()) @@ -453,7 +452,6 @@ FileEntryRef FileManager::getVirtualFileRef(StringRef Filename, off_t Size, UFE->ModTime = ModificationTime; UFE->Dir = &DirInfo->getDirEntry(); UFE->UID = NextFileUID++; - UFE->IsValid = true; UFE->File.reset(); return FileEntryRef(NamedFileEnt); } @@ -483,7 +481,6 @@ llvm::Optional FileManager::getBypassFile(FileEntryRef VF) { BFE->Dir = VF.getFileEntry().Dir; BFE->ModTime = llvm::sys::toTimeT(Status.getLastModificationTime()); BFE->UID = NextFileUID++; - BFE->IsValid = true; // Save the entry in the bypass table and return. return FileEntryRef(*Insertion.first); diff --git a/clang/lib/Frontend/LogDiagnosticPrinter.cpp b/clang/lib/Frontend/LogDiagnosticPrinter.cpp index df8b236..d810b37 100644 --- a/clang/lib/Frontend/LogDiagnosticPrinter.cpp +++ b/clang/lib/Frontend/LogDiagnosticPrinter.cpp @@ -118,8 +118,7 @@ void LogDiagnosticPrinter::HandleDiagnostic(DiagnosticsEngine::Level Level, const SourceManager &SM = Info.getSourceManager(); FileID FID = SM.getMainFileID(); if (FID.isValid()) { - const FileEntry *FE = SM.getFileEntryForID(FID); - if (FE && FE->isValid()) + if (const FileEntry *FE = SM.getFileEntryForID(FID)) MainFilename = std::string(FE->getName()); } } @@ -148,8 +147,7 @@ void LogDiagnosticPrinter::HandleDiagnostic(DiagnosticsEngine::Level Level, // At least print the file name if available: FileID FID = SM.getFileID(Info.getLocation()); if (FID.isValid()) { - const FileEntry *FE = SM.getFileEntryForID(FID); - if (FE && FE->isValid()) + if (const FileEntry *FE = SM.getFileEntryForID(FID)) DE.Filename = std::string(FE->getName()); } } else { diff --git a/clang/lib/Frontend/TextDiagnostic.cpp b/clang/lib/Frontend/TextDiagnostic.cpp index 1c4a76e..6c0ea0c 100644 --- a/clang/lib/Frontend/TextDiagnostic.cpp +++ b/clang/lib/Frontend/TextDiagnostic.cpp @@ -798,8 +798,7 @@ void TextDiagnostic::emitDiagnosticLoc(FullSourceLoc Loc, PresumedLoc PLoc, // At least print the file name if available: FileID FID = Loc.getFileID(); if (FID.isValid()) { - const FileEntry *FE = Loc.getFileEntry(); - if (FE && FE->isValid()) { + if (const FileEntry *FE = Loc.getFileEntry()) { emitFilename(FE->getName(), Loc.getManager()); OS << ": "; } diff --git a/clang/unittests/Basic/FileEntryTest.cpp b/clang/unittests/Basic/FileEntryTest.cpp index 16c8e57..6c070fb 100644 --- a/clang/unittests/Basic/FileEntryTest.cpp +++ b/clang/unittests/Basic/FileEntryTest.cpp @@ -12,25 +12,22 @@ #include "gtest/gtest.h" using namespace llvm; -using namespace clang; -namespace { - -using FileMap = StringMap>; -using DirMap = StringMap>; +namespace clang { -struct RefMaps { - FileMap Files; - DirMap Dirs; +class FileEntryTestHelper { + StringMap> Files; + StringMap> Dirs; SmallVector, 5> FEs; SmallVector, 5> DEs; DirectoryEntryRef DR; - RefMaps() : DR(addDirectory("dir")) {} +public: + FileEntryTestHelper() : DR(addDirectory("dir")) {} DirectoryEntryRef addDirectory(StringRef Name) { - DEs.push_back(std::make_unique()); + DEs.emplace_back(new DirectoryEntry()); return DirectoryEntryRef(*Dirs.insert({Name, *DEs.back()}).first); } DirectoryEntryRef addDirectoryAlias(StringRef Name, DirectoryEntryRef Base) { @@ -40,7 +37,7 @@ struct RefMaps { } FileEntryRef addFile(StringRef Name) { - FEs.push_back(std::make_unique()); + FEs.emplace_back(new FileEntry()); return FileEntryRef( *Files.insert({Name, FileEntryRef::MapValue(*FEs.back().get(), DR)}) .first); @@ -55,19 +52,9 @@ struct RefMaps { } }; -TEST(FileEntryTest, Constructor) { - FileEntry FE; - EXPECT_EQ(0, FE.getSize()); - EXPECT_EQ(0, FE.getModificationTime()); - EXPECT_EQ(nullptr, FE.getDir()); - EXPECT_EQ(0U, FE.getUniqueID().getDevice()); - EXPECT_EQ(0U, FE.getUniqueID().getFile()); - EXPECT_EQ(false, FE.isNamedPipe()); - EXPECT_EQ(false, FE.isValid()); -} - +namespace { TEST(FileEntryTest, FileEntryRef) { - RefMaps Refs; + FileEntryTestHelper Refs; FileEntryRef R1 = Refs.addFile("1"); FileEntryRef R2 = Refs.addFile("2"); FileEntryRef R1Also = Refs.addFileAlias("1-also", R1); @@ -84,7 +71,7 @@ TEST(FileEntryTest, FileEntryRef) { } TEST(FileEntryTest, OptionalFileEntryRefDegradesToFileEntryPtr) { - RefMaps Refs; + FileEntryTestHelper Refs; OptionalFileEntryRefDegradesToFileEntryPtr M0; OptionalFileEntryRefDegradesToFileEntryPtr M1 = Refs.addFile("1"); OptionalFileEntryRefDegradesToFileEntryPtr M2 = Refs.addFile("2"); @@ -102,7 +89,7 @@ TEST(FileEntryTest, OptionalFileEntryRefDegradesToFileEntryPtr) { } TEST(FileEntryTest, equals) { - RefMaps Refs; + FileEntryTestHelper Refs; FileEntryRef R1 = Refs.addFile("1"); FileEntryRef R2 = Refs.addFile("2"); FileEntryRef R1Also = Refs.addFileAlias("1-also", R1); @@ -123,7 +110,7 @@ TEST(FileEntryTest, equals) { } TEST(FileEntryTest, isSameRef) { - RefMaps Refs; + FileEntryTestHelper Refs; FileEntryRef R1 = Refs.addFile("1"); FileEntryRef R2 = Refs.addFile("2"); FileEntryRef R1Also = Refs.addFileAlias("1-also", R1); @@ -135,7 +122,7 @@ TEST(FileEntryTest, isSameRef) { } TEST(FileEntryTest, DenseMapInfo) { - RefMaps Refs; + FileEntryTestHelper Refs; FileEntryRef R1 = Refs.addFile("1"); FileEntryRef R2 = Refs.addFile("2"); FileEntryRef R1Also = Refs.addFileAlias("1-also", R1); @@ -164,7 +151,7 @@ TEST(FileEntryTest, DenseMapInfo) { } TEST(DirectoryEntryTest, isSameRef) { - RefMaps Refs; + FileEntryTestHelper Refs; DirectoryEntryRef R1 = Refs.addDirectory("1"); DirectoryEntryRef R2 = Refs.addDirectory("2"); DirectoryEntryRef R1Also = Refs.addDirectoryAlias("1-also", R1); @@ -176,7 +163,7 @@ TEST(DirectoryEntryTest, isSameRef) { } TEST(DirectoryEntryTest, DenseMapInfo) { - RefMaps Refs; + FileEntryTestHelper Refs; DirectoryEntryRef R1 = Refs.addDirectory("1"); DirectoryEntryRef R2 = Refs.addDirectory("2"); DirectoryEntryRef R1Also = Refs.addDirectoryAlias("1-also", R1); @@ -205,3 +192,4 @@ TEST(DirectoryEntryTest, DenseMapInfo) { } } // end namespace +} // namespace clang diff --git a/clang/unittests/Basic/FileManagerTest.cpp b/clang/unittests/Basic/FileManagerTest.cpp index 3c058a0..31cb2bb 100644 --- a/clang/unittests/Basic/FileManagerTest.cpp +++ b/clang/unittests/Basic/FileManagerTest.cpp @@ -165,7 +165,6 @@ TEST_F(FileManagerTest, getFileReturnsValidFileEntryForExistingRealFile) { auto file = manager.getFile("/tmp/test"); ASSERT_TRUE(file); - ASSERT_TRUE((*file)->isValid()); EXPECT_EQ("/tmp/test", (*file)->getName()); const DirectoryEntry *dir = (*file)->getDir(); @@ -190,7 +189,6 @@ TEST_F(FileManagerTest, getFileReturnsValidFileEntryForExistingVirtualFile) { manager.getVirtualFile("virtual/dir/bar.h", 100, 0); auto file = manager.getFile("virtual/dir/bar.h"); ASSERT_TRUE(file); - ASSERT_TRUE((*file)->isValid()); EXPECT_EQ("virtual/dir/bar.h", (*file)->getName()); const DirectoryEntry *dir = (*file)->getDir(); @@ -212,9 +210,7 @@ TEST_F(FileManagerTest, getFileReturnsDifferentFileEntriesForDifferentFiles) { auto fileFoo = manager.getFile("foo.cpp"); auto fileBar = manager.getFile("bar.cpp"); ASSERT_TRUE(fileFoo); - ASSERT_TRUE((*fileFoo)->isValid()); ASSERT_TRUE(fileBar); - ASSERT_TRUE((*fileBar)->isValid()); EXPECT_NE(*fileFoo, *fileBar); } @@ -341,9 +337,6 @@ TEST_F(FileManagerTest, getFileReturnsSameFileEntryForAliasedVirtualFiles) { statCache->InjectFile("abc/bar.cpp", 42); manager.setStatCache(std::move(statCache)); - ASSERT_TRUE(manager.getVirtualFile("abc/foo.cpp", 100, 0)->isValid()); - ASSERT_TRUE(manager.getVirtualFile("abc/bar.cpp", 200, 0)->isValid()); - auto f1 = manager.getFile("abc/foo.cpp"); auto f2 = manager.getFile("abc/bar.cpp"); @@ -418,14 +411,12 @@ TEST_F(FileManagerTest, getVirtualFileWithDifferentName) { // Inject the virtual file: const FileEntry *file1 = manager.getVirtualFile("c:\\tmp\\test", 123, 1); ASSERT_TRUE(file1 != nullptr); - ASSERT_TRUE(file1->isValid()); EXPECT_EQ(43U, file1->getUniqueID().getFile()); EXPECT_EQ(123, file1->getSize()); // Lookup the virtual file with a different name: auto file2 = manager.getFile("c:/tmp/test", 100, 1); ASSERT_TRUE(file2); - ASSERT_TRUE((*file2)->isValid()); // Check that it's the same UFE: EXPECT_EQ(file1, *file2); EXPECT_EQ(43U, (*file2)->getUniqueID().getFile()); @@ -487,7 +478,6 @@ TEST_F(FileManagerTest, getVirtualFileFillsRealPathName) { // Check for real path. const FileEntry *file = Manager.getVirtualFile("/tmp/test", 123, 1); ASSERT_TRUE(file != nullptr); - ASSERT_TRUE(file->isValid()); SmallString<64> ExpectedResult = CustomWorkingDir; llvm::sys::path::append(ExpectedResult, "tmp", "test"); @@ -515,7 +505,6 @@ TEST_F(FileManagerTest, getFileDontOpenRealPath) { // Check for real path. auto file = Manager.getFile("/tmp/test", /*OpenFile=*/false); ASSERT_TRUE(file); - ASSERT_TRUE((*file)->isValid()); SmallString<64> ExpectedResult = CustomWorkingDir; llvm::sys::path::append(ExpectedResult, "tmp", "test"); @@ -548,7 +537,6 @@ TEST_F(FileManagerTest, getBypassFile) { const FileEntry *File = Manager.getVirtualFile("/tmp/test", /*Size=*/10, 0); ASSERT_TRUE(File); const FileEntry &FE = *File; - EXPECT_TRUE(FE.isValid()); EXPECT_EQ(FE.getSize(), 10); // Calling a second time should not affect the UID or size. @@ -564,7 +552,6 @@ TEST_F(FileManagerTest, getBypassFile) { llvm::Optional BypassRef = Manager.getBypassFile(File->getLastRef()); ASSERT_TRUE(BypassRef); - EXPECT_TRUE(BypassRef->isValid()); EXPECT_EQ("/tmp/test", BypassRef->getName()); // Check that it's different in the right ways. -- 2.7.4