Skip to content

Commit

Permalink
Remove a few effectively-unused FileEntry APIs. NFC
Browse files Browse the repository at this point in the history 
- 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
  • Loading branch information
@sam-mccall
sam-mccall committed Apr 7, 2022
1 parent dbf35b7 commit b2a7f1c
Show file tree
Hide file tree
Showing 7 changed files with 28 additions and 61 deletions.
4 changes: 4 additions & 0 deletions clang/include/clang/Basic/DirectoryEntry.h
View file
Expand Up @@ -32,7 +32,11 @@ template <class RefTy> 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.
Expand Down
14 changes: 4 additions & 10 deletions clang/include/clang/Basic/FileEntry.h
View file
Expand Up @@ -65,7 +65,6 @@ class FileEntryRef {
}
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;
Expand Down Expand Up @@ -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.
Expand All @@ -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<llvm::vfs::File> File;
Expand All @@ -355,17 +357,11 @@ class FileEntry {
Optional<FileEntryRef> 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; }
Expand All @@ -381,8 +377,6 @@ class FileEntry {
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(); }
Expand Down
3 changes: 0 additions & 3 deletions clang/lib/Basic/FileManager.cpp
View file
Expand Up @@ -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())
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -483,7 +481,6 @@ llvm::Optional<FileEntryRef> 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);
Expand Down
6 changes: 2 additions & 4 deletions clang/lib/Frontend/LogDiagnosticPrinter.cpp
View file
Expand Up @@ -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());
}
}
Expand Down Expand Up @@ -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 {
Expand Down
3 changes: 1 addition & 2 deletions clang/lib/Frontend/TextDiagnostic.cpp
View file
Expand Up @@ -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 << ": ";
}
Expand Down
46 changes: 17 additions & 29 deletions clang/unittests/Basic/FileEntryTest.cpp
View file
Expand Up @@ -12,25 +12,22 @@
#include "gtest/gtest.h"

using namespace llvm;
using namespace clang;

namespace {

using FileMap = StringMap<llvm::ErrorOr<FileEntryRef::MapValue>>;
using DirMap = StringMap<llvm::ErrorOr<DirectoryEntry &>>;
namespace clang {

struct RefMaps {
FileMap Files;
DirMap Dirs;
class FileEntryTestHelper {
StringMap<llvm::ErrorOr<FileEntryRef::MapValue>> Files;
StringMap<llvm::ErrorOr<DirectoryEntry &>> Dirs;

SmallVector<std::unique_ptr<FileEntry>, 5> FEs;
SmallVector<std::unique_ptr<DirectoryEntry>, 5> DEs;
DirectoryEntryRef DR;

RefMaps() : DR(addDirectory("dir")) {}
public:
FileEntryTestHelper() : DR(addDirectory("dir")) {}

DirectoryEntryRef addDirectory(StringRef Name) {
DEs.push_back(std::make_unique<DirectoryEntry>());
DEs.emplace_back(new DirectoryEntry());
return DirectoryEntryRef(*Dirs.insert({Name, *DEs.back()}).first);
}
DirectoryEntryRef addDirectoryAlias(StringRef Name, DirectoryEntryRef Base) {
Expand All @@ -40,7 +37,7 @@ struct RefMaps {
}

FileEntryRef addFile(StringRef Name) {
FEs.push_back(std::make_unique<FileEntry>());
FEs.emplace_back(new FileEntry());
return FileEntryRef(
*Files.insert({Name, FileEntryRef::MapValue(*FEs.back().get(), DR)})
.first);
Expand All @@ -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);
Expand All @@ -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");
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -205,3 +192,4 @@ TEST(DirectoryEntryTest, DenseMapInfo) {
}

} // end namespace
} // namespace clang
13 changes: 0 additions & 13 deletions clang/unittests/Basic/FileManagerTest.cpp
View file
Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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);
}

Expand Down Expand Up @@ -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");

Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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.
Expand All @@ -564,7 +552,6 @@ TEST_F(FileManagerTest, getBypassFile) {
llvm::Optional<FileEntryRef> 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.
Expand Down

0 comments on commit b2a7f1c

Please sign in to comment.