sammccall created this revision.
sammccall added reviewers: dexonsmith, kadircet.
Herald added a project: All.
sammccall requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
- isValid: FileManager only ever returns valid FileEntries (see next point)
- construction from outside FileManager (also DirectoryEntry). It's not
possible to create a useful FileEntry this way, there are no setters. This was
only used in FileEntryTest.
- operator< (hypothetical callers who want to sort FileEntry*s by UID should
probably be explicit about it).
The ugly part here:
- FileEntryTest was constructing dummy FileEntrys to wrap in FileEntryRef
- I changed this to use a FileManager as a factory
- but FileManager hands out *const* FileEntry*s
- so I needed to change FileEntryRef to wrap *const* FileEntry&
Fortunately this doesn't change the public APIs around FileEntryRef.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D123197
Files:
clang/include/clang/Basic/DirectoryEntry.h
clang/include/clang/Basic/FileEntry.h
clang/include/clang/Basic/FileManager.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
@@ -7,8 +7,11 @@
//===----------------------------------------------------------------------===//
#include "clang/Basic/FileEntry.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/VirtualFileSystem.h"
#include "gtest/gtest.h"
using namespace llvm;
@@ -17,21 +20,25 @@
namespace {
using FileMap = StringMap<llvm::ErrorOr<FileEntryRef::MapValue>>;
-using DirMap = StringMap<llvm::ErrorOr<DirectoryEntry &>>;
+using DirMap = StringMap<llvm::ErrorOr<const DirectoryEntry &>>;
struct RefMaps {
+ FileManager FM; // Used only to create FileEntry/DirectoryEntry.
+ unsigned UniqueNameCounter = 0;
FileMap Files;
DirMap Dirs;
- SmallVector<std::unique_ptr<FileEntry>, 5> FEs;
- SmallVector<std::unique_ptr<DirectoryEntry>, 5> DEs;
DirectoryEntryRef DR;
- RefMaps() : DR(addDirectory("dir")) {}
+ RefMaps()
+ : FM(FileSystemOptions(), std::make_unique<vfs::InMemoryFileSystem>()),
+ DR(addDirectory("dir")) {}
DirectoryEntryRef addDirectory(StringRef Name) {
- DEs.push_back(std::make_unique<DirectoryEntry>());
- return DirectoryEntryRef(*Dirs.insert({Name, *DEs.back()}).first);
+ const DirectoryEntry *DE =
+ FM.getVirtualFile(std::to_string(UniqueNameCounter++) + "/x", 0, 0)
+ ->getDir();
+ return DirectoryEntryRef(*Dirs.insert({Name, *DE}).first);
}
DirectoryEntryRef addDirectoryAlias(StringRef Name, DirectoryEntryRef Base) {
return DirectoryEntryRef(
@@ -40,10 +47,10 @@
}
FileEntryRef addFile(StringRef Name) {
- FEs.push_back(std::make_unique<FileEntry>());
+ const FileEntry *FE =
+ FM.getVirtualFile(std::to_string(UniqueNameCounter++), 0, 0);
return FileEntryRef(
- *Files.insert({Name, FileEntryRef::MapValue(*FEs.back().get(), DR)})
- .first);
+ *Files.insert({Name, FileEntryRef::MapValue(*FE, DR)}).first);
}
FileEntryRef addFileAlias(StringRef Name, FileEntryRef Base) {
return FileEntryRef(
@@ -55,17 +62,6 @@
}
};
-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());
-}
-
TEST(FileEntryTest, FileEntryRef) {
RefMaps Refs;
FileEntryRef R1 = Refs.addFile("1");
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
@@ -215,7 +215,7 @@
// Construct and return and FileEntryRef, unless it's a redirect to another
// filename.
FileEntryRef::MapValue Value = *SeenFileInsertResult.first->second;
- if (LLVM_LIKELY(Value.V.is<FileEntry *>()))
+ if (LLVM_LIKELY(Value.V.is<const FileEntry *>()))
return FileEntryRef(*SeenFileInsertResult.first);
return FileEntryRef(*reinterpret_cast<const FileEntryRef::MapEntry *>(
Value.V.get<const void *>()));
@@ -301,9 +301,9 @@
*SeenFileEntries
.insert({Status.getName(), FileEntryRef::MapValue(*UFE, DirInfo)})
.first;
- assert(Redirection.second->V.is<FileEntry *>() &&
+ assert(Redirection.second->V.is<const FileEntry *>() &&
"filename redirected to a non-canonical filename?");
- assert(Redirection.second->V.get<FileEntry *>() == UFE &&
+ assert(Redirection.second->V.get<const FileEntry *>() == UFE &&
"filename from getStatValue() refers to wrong file");
// Cache the redirection in the previously-inserted entry, still available
@@ -350,7 +350,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())
@@ -395,7 +394,7 @@
{Filename, std::errc::no_such_file_or_directory}).first;
if (NamedFileEnt.second) {
FileEntryRef::MapValue Value = *NamedFileEnt.second;
- if (LLVM_LIKELY(Value.V.is<FileEntry *>()))
+ if (LLVM_LIKELY(Value.V.is<const FileEntry *>()))
return FileEntryRef(NamedFileEnt);
return FileEntryRef(*reinterpret_cast<const FileEntryRef::MapEntry *>(
Value.V.get<const void *>()));
@@ -460,8 +459,7 @@
UFE->Size = Size;
UFE->ModTime = ModificationTime;
UFE->Dir = &DirInfo->getDirEntry();
- UFE->UID = NextFileUID++;
- UFE->IsValid = true;
+ UFE->UID = NextFileUID++;
UFE->File.reset();
return FileEntryRef(NamedFileEnt);
}
@@ -491,7 +489,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);
@@ -619,7 +616,7 @@
FEEnd = SeenFileEntries.end();
FE != FEEnd; ++FE)
if (llvm::ErrorOr<FileEntryRef::MapValue> Entry = FE->getValue()) {
- if (const auto *FE = Entry->V.dyn_cast<FileEntry *>())
+ if (const auto *FE = Entry->V.dyn_cast<const FileEntry *>())
UIDToFiles[FE->getUID()] = FE;
}
Index: clang/include/clang/Basic/FileManager.h
===================================================================
--- clang/include/clang/Basic/FileManager.h
+++ clang/include/clang/Basic/FileManager.h
@@ -83,8 +83,8 @@
/// for virtual directories/files are owned by
/// VirtualDirectoryEntries/VirtualFileEntries above.
///
- llvm::StringMap<llvm::ErrorOr<DirectoryEntry &>, llvm::BumpPtrAllocator>
- SeenDirEntries;
+ llvm::StringMap<llvm::ErrorOr<const DirectoryEntry &>, llvm::BumpPtrAllocator>
+ SeenDirEntries;
/// A cache that maps paths to file entries (either real or
/// virtual) we have looked up, or an error that occurred when we looked up
Index: clang/include/clang/Basic/FileEntry.h
===================================================================
--- clang/include/clang/Basic/FileEntry.h
+++ clang/include/clang/Basic/FileEntry.h
@@ -61,11 +61,10 @@
public:
StringRef getName() const { return ME->first(); }
const FileEntry &getFileEntry() const {
- return *ME->second->V.get<FileEntry *>();
+ return *ME->second->V.get<const FileEntry *>();
}
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;
@@ -115,13 +114,13 @@
///
/// The second type is really a `const MapEntry *`, but that confuses
/// gcc5.3. Once that's no longer supported, change this back.
- llvm::PointerUnion<FileEntry *, const void *> V;
+ llvm::PointerUnion<const FileEntry *, const void *> V;
/// Directory the file was found in. Set if and only if V is a FileEntry.
Optional<DirectoryEntryRef> Dir;
MapValue() = delete;
- MapValue(FileEntry &FE, DirectoryEntryRef Dir) : V(&FE), Dir(Dir) {}
+ MapValue(const FileEntry &FE, DirectoryEntryRef Dir) : V(&FE), Dir(Dir) {}
MapValue(MapEntry &ME) : V(&ME) {}
};
@@ -151,7 +150,7 @@
explicit FileEntryRef(const MapEntry &ME) : ME(&ME) {
assert(ME.second && "Expected payload");
assert(ME.second->V && "Expected non-null");
- assert(ME.second->V.is<FileEntry *>() && "Expected FileEntry");
+ assert(ME.second->V.is<const FileEntry *>() && "Expected FileEntry");
}
/// Expose the underlying MapEntry to simplify packing in a PointerIntPair or
@@ -330,6 +329,9 @@
/// descriptor for the file.
class FileEntry {
friend class FileManager;
+ 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 +340,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 +356,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; }
@@ -374,8 +369,6 @@
/// Return the directory the file lives in.
const DirectoryEntry *getDir() const { return Dir; }
- bool operator<(const FileEntry &RHS) const { return UniqueID < RHS.UniqueID; }
-
/// Check whether the file is a named pipe (and thus can't be opened by
/// the native FileManager methods).
bool isNamedPipe() const { return IsNamedPipe; }
@@ -383,8 +376,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,6 +32,9 @@
/// 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;
// FIXME: We should not be storing a directory entry name here.
@@ -55,7 +58,7 @@
return llvm::hash_value(&Ref.getDirEntry());
}
- using MapEntry = llvm::StringMapEntry<llvm::ErrorOr<DirectoryEntry &>>;
+ using MapEntry = llvm::StringMapEntry<llvm::ErrorOr<const DirectoryEntry &>>;
const MapEntry &getMapEntry() const { return *ME; }
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
