sammccall updated this revision to Diff 421021.
sammccall added a comment.
Make test a friend of {File,Directory}Entry to avoid the FileManager-as-factory.
(A real constructor would be cleaner, but requires tricky FileManager changes).
Revert most of the related test changes. Had to rename the test helper so the
friend declaration is clearer.
Revert const changes. Test no longer needs them. I'm not sure they're better.
Remove operator< change which I landed separately.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123197/new/
https://reviews.llvm.org/D123197
Files:
clang/include/clang/Basic/DirectoryEntry.h
clang/include/clang/Basic/FileEntry.h
clang/lib/Basic/FileManager.cpp
clang/lib/Frontend/LogDiagnosticPrinter.cpp
clang/lib/Frontend/TextDiagnostic.cpp
clang/unittests/Basic/FileEntryTest.cpp
clang/unittests/Basic/FileManagerTest.cpp
Index: clang/unittests/Basic/FileManagerTest.cpp
===================================================================
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -165,7 +165,6 @@
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 @@
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 @@
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 @@
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 @@
// 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 @@
// 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 @@
// 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 @@
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 @@
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.
Index: clang/unittests/Basic/FileEntryTest.cpp
===================================================================
--- clang/unittests/Basic/FileEntryTest.cpp
+++ clang/unittests/Basic/FileEntryTest.cpp
@@ -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) {
@@ -40,7 +37,7 @@
}
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);
@@ -55,19 +52,9 @@
}
};
-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, OptionalFileEntryRefDegradesToFileEntryPtr) {
- RefMaps Refs;
+ FileEntryTestHelper Refs;
OptionalFileEntryRefDegradesToFileEntryPtr M0;
OptionalFileEntryRefDegradesToFileEntryPtr M1 = Refs.addFile("1");
OptionalFileEntryRefDegradesToFileEntryPtr M2 = Refs.addFile("2");
@@ -102,7 +89,7 @@
}
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, 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, 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(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, 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 @@
}
} // end namespace
+} // namespace clang
Index: clang/lib/Frontend/TextDiagnostic.cpp
===================================================================
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -798,8 +798,7 @@
// 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 << ": ";
}
Index: clang/lib/Frontend/LogDiagnosticPrinter.cpp
===================================================================
--- clang/lib/Frontend/LogDiagnosticPrinter.cpp
+++ clang/lib/Frontend/LogDiagnosticPrinter.cpp
@@ -118,8 +118,7 @@
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 @@
// 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 {
Index: clang/lib/Basic/FileManager.cpp
===================================================================
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -342,7 +342,6 @@
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 @@
UFE->ModTime = ModificationTime;
UFE->Dir = &DirInfo->getDirEntry();
UFE->UID = NextFileUID++;
- UFE->IsValid = true;
UFE->File.reset();
return FileEntryRef(NamedFileEnt);
}
@@ -483,7 +481,6 @@
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);
Index: clang/include/clang/Basic/FileEntry.h
===================================================================
--- clang/include/clang/Basic/FileEntry.h
+++ clang/include/clang/Basic/FileEntry.h
@@ -65,7 +65,6 @@
}
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 @@
/// 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 @@
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;
@@ -355,17 +357,11 @@
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; }
@@ -381,8 +377,6 @@
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(); }
Index: clang/include/clang/Basic/DirectoryEntry.h
===================================================================
--- clang/include/clang/Basic/DirectoryEntry.h
+++ clang/include/clang/Basic/DirectoryEntry.h
@@ -32,7 +32,11 @@
/// 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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
