Remove a few effectively-unused FileEntry APIs. NFC

[sammccall]

Summary

• 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.

[kadircet]

thanks lgtm! I would still be looking out for build bot statuses in case there are OS specific code paths that were using isvalid. it might also be worthwhile to send out operator< change in a separate commit, just to give downstream users an opportunity to handle fixes for the two in isolation (or reverts if need be).

clang/lib/Basic/FileManager.cpp

462–463

nit: revert formatting

clang/unittests/Basic/FileEntryTest.cpp

43–44

no need for const_cast here anymore, right?

56

similarly we can drop the const_cast here

[sammccall]

Remove obsolete const_casts

[sammccall]

In D123197#3432386, @kadircet wrote:

thanks lgtm! I would still be looking out for build bot statuses in case there are OS specific code paths that were using isvalid.

Will do!

it might also be worthwhile to send out operator< change in a separate commit, just to give downstream users an opportunity to handle fixes for the two in isolation (or reverts if need be).

I think it's so unlikely to be used as to not be worth it.

• It's not possible to put FileEntrys as values in a container, so some code would have to be explicitly comparing them (rather than std::sort or so).
• Ordering file entries by UID doesn't seem terribly useful - it's neither the cheapest ordering (pointers) nor the most natural.
• Empirically, there are zero uses in llvm-project and zero in google's downstream repo, which is one of the heavier user of clang APIs. I'd bet there are none anywhere.

clang/lib/Basic/FileManager.cpp

462–463

arc insists on this change here, because it's in the blast radius of the next line and clang-format isn't configured to allow this formatting.

Unless you feel strongly I'd rather not fight the linter when it's not really wrong.
(Happy to reformat the other lines to match though)

[kadircet]

clang/lib/Basic/FileManager.cpp

462–463

Unless you feel strongly I'd rather not fight the linter when it's not really wrong.
(Happy to reformat the other lines to match though)

I was suggesting this for preserving history purposes, in theory all of this is old code with intricate semantics so it would be nice to not add more commits to the history chain here. I think it should be possible to just arc diff --nolint (or push without updating the diff in phabricator, which you already probably know, but just in case).

[sammccall]

revert clang-format changes

[sammccall]

clang/lib/Basic/FileManager.cpp

462–463

Sure, fair enough

[dexonsmith]

The ugly part here:

• FileEntryTest was constructing dummy FileEntrys to wrap in FileEntryRef
• I changed this to use a FileManager as a factory

It's not clear *why* you changed this to use a FileManager as a factory. It seems unrelated to removing FileEntry::isValid() (but maybe I'm missing something obvious). It also doesn't seem like an improvement for the test, since FileManager is full of hacks and twisted logic that's hard to reason about, and it's useful to test FileEntry / FileEntryRef / etc. in isolation of that.

Depending on the goal, maybe instead we could abstract out some lower-level API, FileManagerBase or FileEntryFactory or something, which both FileEntryTest and FileManager use.

clang/unittests/Basic/FileEntryTest.cpp

47–52

I don't love this from a layering perspective. FileEntryTest is testing the low-level pieces that are used inside FileManager... using FileManager to generate the FileEntries is awkward.

Maybe it'd be okay if FileManager::getVirtualFile() were trivial, but it's not.

[dexonsmith]

Adding a couple of reviewers that have been looking at FileManager issues with me recently; maybe they have a different perspective.

(also, I neglected to say so explicitly before: removing isValid() SGTM; FileManager clients shouldn't have to worry about it)

[dexonsmith]

clang/lib/Basic/FileManager.cpp

462–463

arc insists on this change here, because it's in the blast radius of the next line and clang-format isn't configured to allow this formatting.

Unless you feel strongly I'd rather not fight the linter when it's not really wrong.
(Happy to reformat the other lines to match though)

I know this was already reverted / dealt with (SGTM; I agree with @kadircet, although probably I feel less strongly), but I'm wondering if there's a bug in arc or clang-format to report or fix. Seems to me like this *shouldn't* be considered part of the blast radius. Why should deleting a line trigger reformatting the lines before/after, when those lines are not part of the same construct/statement?

[dexonsmith]

clang/include/clang/Basic/FileEntry.h

117

These const-ness changes look independent and could land separately / ahead of time as NFC changes, independently of isValid() and whatever we end up with for FileEntryTest.

[sammccall]

In D123197#3433755, @dexonsmith wrote:

The ugly part here:

• FileEntryTest was constructing dummy FileEntrys to wrap in FileEntryRef
• I changed this to use a FileManager as a factory

It's not clear *why* you changed this to use a FileManager as a factory. It seems unrelated to removing FileEntry::isValid() (but maybe I'm missing something obvious).

I removed the public default constructor of FileEntry, which was the one remaining way to create an invalid FileEntry.

Such a FileEntry is almost entirely useless, because it can never be put into a valid state and isn't useful as a sentinel value. However FileEntryTest does use them, being careful to only rely on their address.

It also doesn't seem like an improvement for the test, since FileManager is full of hacks and twisted logic that's hard to reason about

Agree. The test is suffering here in order to achieve a clean public API while keeping the basic structure of the test the same as before.

and it's useful to test FileEntry / FileEntryRef / etc. in isolation of that.

Yes, though the FileManager design doesn't support that very well. FileEntryRef method fields in FileEntry which can only be set by FileManager, so isolating the test means those can't be tested at all.

Depending on the goal, maybe instead we could abstract out some lower-level API, FileManagerBase or FileEntryFactory or something, which both FileEntryTest and FileManager use.

My goal is to make the API simpler to understand by eliminating states/APIs that aren't relevant to production code and confuse readers.

I think it's worth making the test a bit uglier to achieve that, and by contrast adding inheritance to FileManager would make it harder to understand.

Some ideas, do any seem acceptable?

• have FileEntry friend class FileEntryTest or so
• add static FileEntry FileEntry::createForTest() to replace the public default constructor
• change this test to cover FM/FE/FER together
• use FM as a factory as in this patch, but document it verbosely
• make the constructor of FileEntry protected instead of private, and define a subclass for tests

[benlangmuir]

clang/unittests/Basic/FileEntryTest.cpp

47–52

Could we add friend class FileEntryTest to FileEntry to get access to the private constructor and go back to the original approach? I see some other places in llvm that use that pattern.

[benlangmuir]

clang/unittests/Basic/FileEntryTest.cpp

47–52

(just saw Sam made the same suggestion)

[dexonsmith]

clang/include/clang/Basic/FileEntry.h

332–334

Aha, now I understand why you needed a factory in the FileEntryTest: because you made the constructor private.

Instead, can we add an initializing constructor to FileEntry? It'd be fine for that to be public; it seems implausible for someone to misuse it.

clang/lib/Basic/FileManager.cpp

422–462

This dance'll be a bit different if there's an initializing constructor, but maybe it'd be cleaner anyway:

FileEntry **Slot = nullptr;
if (!getStat...) {
  Status = ...; // and update name in Status with fillRealPathName()
  Slot = &UniqueRealFiles[Status.getUniqueID()];
  if (*Slot) return early;
} else {
  Status = ...;
  VirtualFileEntries.push_back(nullptr);
  Slot = &VirtualFileEntries.back();
}
UFE = *Slot = new (FilesAlloc.Allocate()) FileEntry(Status, Dir, NextFileUID++);
NamedFileEnt.second = FileEntryRef::MapValue(*UFE, *DirInfo);
UFE->LastRef = UFE;

[sammccall]

clang/include/clang/Basic/FileEntry.h

117

Sure. In it's current form the patch requires these changes as we can't get a mutable FileEntry*.

I'll split them out.

clang/lib/Basic/FileManager.cpp

462–463

I happen to know this one :-)
Clang-format can support alignment between lines (e.g. of =). Removing line 2 from well-formatted code *can* cause spaces to need to be removed from line 1.
In this case, AlignConsecutiveAssignments is false in LLVM style, and the spaces were removed for an unrelated reason. But clang-format doesn't track causality. Getting it to preserve misformatting adjacent to modified code would be a bit of a project!

[dexonsmith]

(Seeing the other replies now!)

In D123197#3433846, @sammccall wrote:

I think it's worth making the test a bit uglier to achieve that, and by contrast adding inheritance to FileManager would make it harder to understand.

Some ideas, do any seem acceptable?

• have FileEntry friend class FileEntryTest or so

This idea sounds good to me.

I also like the idea I just posted (change to an initializing constructor), but it's more work and I'm fine with the friend class FileEntryTest as a way forward if you'd prefer it.

[sammccall]

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.

[dexonsmith]

LGTM; thanks for iterating on it.

clang/include/clang/Basic/DirectoryEntry.h

 
 template <class RefTy> class MapEntryOptionalStorage;
 
 } // end namespace FileMgr
 
 /// 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.
   StringRef Name; // Name of the directory.
 
 public:
   StringRef getName() const { return Name; }
 };
 
 /// A reference to a \c DirectoryEntry  that includes the name of the directory
 /// as it was accessed by the FileManager's client.
 class DirectoryEntryRef {
 public:
   const DirectoryEntry &getDirEntry() const { return *ME->getValue(); }
 
   StringRef getName() const { return ME->getKey(); }
 
   /// Hash code is based on the DirectoryEntry, not the specific named
   /// reference.
   friend llvm::hash_code hash_value(DirectoryEntryRef Ref) {
     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; }
 
   /// Check if RHS referenced the file in exactly the same way.
   bool isSameRef(DirectoryEntryRef RHS) const { return ME == RHS.ME; }
 
   DirectoryEntryRef() = delete;
   DirectoryEntryRef(const MapEntry &ME) : ME(&ME) {}

clang/include/clang/Basic/FileEntry.h

 class FileEntry;
 
 /// A reference to a \c FileEntry that includes the name of the file as it was
 /// accessed by the FileManager's client.
 class FileEntryRef {
 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;
   inline time_t getModificationTime() const;
   inline bool isNamedPipe() const;
   inline void closeFile() const;
 
   /// Check if the underlying FileEntry is the same, intentially ignoring
struct MapValue {
     /// The pointer at another MapEntry is used when the FileManager should
     /// silently forward from one name to another, which occurs in Redirecting
     /// VFSs that use external names. In that case, the \c FileEntryRef
     /// returned by the \c FileManager will have the external name, and not the
     /// name that was used to lookup the file.
     ///
     /// 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) {}
   };
 
   /// Check if RHS referenced the file in exactly the same way.
   bool isSameRef(const FileEntryRef &RHS) const { return ME == RHS.ME; }
 
   /// Allow FileEntryRef to degrade into 'const FileEntry*' to facilitate
   /// incremental adoption.
public:
   /// FIXME: Once FileEntryRef is "everywhere" and FileEntry::LastRef and
   /// FileEntry::getName have been deleted, delete this implicit conversion.
   operator const FileEntry *() const { return &getFileEntry(); }
 
   FileEntryRef() = delete;
   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
   /// PointerUnion and allow construction in Optional.
   const clang::FileEntryRef::MapEntry &getMapEntry() const { return *ME; }
 
 private:
   friend class FileMgr::MapEntryOptionalStorage<FileEntryRef>;

 
 /// Cached information about one file (either on disk
 /// or in the virtual file system).
 ///
 /// If the 'File' member is valid, then this FileEntry has an open file
 /// 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.
   time_t ModTime = 0;         // Modification time of file.
   const DirectoryEntry *Dir = nullptr; // Directory file lives in.
   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;
 
   /// The file content, if it is owned by the \p FileEntry.
   std::unique_ptr<llvm::MemoryBuffer> Content;
 
   // First access name for this FileEntry.
   //
   // This is Optional only to allow delayed construction (FileEntryRef has no
   // default constructor). It should always have a value in practice.
   //
   // TODO: remove this once everyone that needs a name uses FileEntryRef.
   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; }
   time_t getModificationTime() const { return ModTime; }
 
   /// 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; }
 
   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(); }
 
 const llvm::sys::fs::UniqueID &FileEntryRef::getUniqueID() const {
   return getFileEntry().getUniqueID();
 }
 

clang/include/clang/Basic/FileManager.h

class FileManager : public RefCountedBase<FileManager> {
   /// virtual) we have looked up, or an error that occurred when we looked up
   /// the directory.
   ///
   /// The actual Entries for real directories/files are
   /// owned by UniqueRealDirs/UniqueRealFiles above, while the Entries
   /// for virtual directories/files are owned by
   /// VirtualDirectoryEntries/VirtualFileEntries above.
   ///
-  llvm::StringMap<llvm::ErrorOr<DirectoryEntry &>, llvm::BumpPtrAllocator>
+  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
   /// the file.
   ///
   /// \see SeenDirEntries
   llvm::StringMap<llvm::ErrorOr<FileEntryRef::MapValue>, llvm::BumpPtrAllocator>
       SeenFileEntries;

clang/lib/Basic/FileManager.cpp

auto SeenFileInsertResult =
       SeenFileEntries.insert({Filename, std::errc::no_such_file_or_directory});
   if (!SeenFileInsertResult.second) {
     if (!SeenFileInsertResult.first->second)
       return llvm::errorCodeToError(
           SeenFileInsertResult.first->second.getError());
     // 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 *>()));
   }
 
   // We've not seen this before. Fill it in.
   ++NumFileCacheMisses;
   auto *NamedFileEnt = &*SeenFileInsertResult.first;
if (Status.getName() == Filename) {
     // implement the FIXME on `ExposesExternalVFSPath`, ie. update the
     // `FileEntryRef::getName()` path to *always* be the virtual path and have
     // clients request the external path only when required through a separate
     // API.
     auto &Redirection =
         *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
     // in the tentative return value.
     NamedFileEnt->second = FileEntryRef::MapValue(Redirection);
 
     // Fix the tentative return value.
     NamedFileEnt = &Redirection;
FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
   UFE->LastRef = ReturnedRef;
   UFE->Size = Status.getSize();
   UFE->ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
   UFE->Dir = &DirInfo.getDirEntry();
   UFE->UID = NextFileUID++;
   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())
       fillRealPathName(UFE, *PathName);
   } else if (!openFile) {
     // We should still fill the path even if we aren't opening the file.
     fillRealPathName(UFE, InterndFileName);
   }
FileEntryRef FileManager::getVirtualFileRef(StringRef Filename, off_t Size,
                                             time_t ModificationTime) {
   ++NumFileLookups;
 
   // See if there is already an entry in the map for an existing file.
   auto &NamedFileEnt = *SeenFileEntries.insert(
       {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 *>()));
   }
 
   // We've not seen this before, or the file is cached as non-existent.
   ++NumFileCacheMisses;
   addAncestorsAsVirtualDirs(Filename);
   FileEntry *UFE = nullptr;
 
   // Now that all ancestors of Filename are in the cache, the
   // following call is guaranteed to find the DirectoryEntry from the
   // cache. A virtual file can also have an empty filename, that could come
   // from a source location preprocessor directive with an empty filename as
   // an example, so we need to pretend it has a name to ensure a valid directory
   // entry can be returned.
   auto DirInfo = expectedToOptional(getDirectoryFromFile(
       *this, Filename.empty() ? "." : Filename, /*CacheFailure=*/true));
   assert(DirInfo &&
          "The directory of a virtual file should already be in the cache.");
 
   // Check to see if the file exists. If so, drop the virtual file
   llvm::vfs::Status Status;
   const char *InterndFileName = NamedFileEnt.first().data();
   if (!getStatValue(InterndFileName, Status, true, nullptr)) {
     Status = llvm::vfs::Status(
       Status.getName(), Status.getUniqueID(),
       llvm::sys::toTimePoint(ModificationTime),
       Status.getUser(), Status.getGroup(), Size,
       Status.getType(), Status.getPermissions());
 
     auto &RealFE = UniqueRealFiles[Status.getUniqueID()];
     if (RealFE) {
       // If we had already opened this file, close it now so we don't
       // leak the descriptor. We're not going to use the file
       // descriptor anyway, since this is a virtual file.
       if (RealFE->File)
         RealFE->closeFile();
       // If we already have an entry with this inode, return it.
       //
       // FIXME: Surely this should add a reference by the new name, and return
       // it instead...
       NamedFileEnt.second = FileEntryRef::MapValue(*RealFE, *DirInfo);
       return FileEntryRef(NamedFileEnt);
     }
     // File exists, but no entry - create it.
     RealFE = new (FilesAlloc.Allocate()) FileEntry();
     RealFE->UniqueID = Status.getUniqueID();
     RealFE->IsNamedPipe =
         Status.getType() == llvm::sys::fs::file_type::fifo_file;
     fillRealPathName(RealFE, Status.getName());
 
     UFE = RealFE;
   } else {
     // File does not exist, create a virtual entry.
     UFE = new (FilesAlloc.Allocate()) FileEntry();
     VirtualFileEntries.push_back(UFE);
   }
 
   NamedFileEnt.second = FileEntryRef::MapValue(*UFE, *DirInfo);
   UFE->LastRef = FileEntryRef(NamedFileEnt);
   UFE->Size    = Size;
   UFE->ModTime = ModificationTime;
   UFE->Dir     = &DirInfo->getDirEntry();
   UFE->UID     = NextFileUID++;
-  UFE->IsValid = true;
   UFE->File.reset();
   return FileEntryRef(NamedFileEnt);
 }
 
 llvm::Optional<FileEntryRef> FileManager::getBypassFile(FileEntryRef VF) {
   // Stat of the file and return nullptr if it doesn't exist.
   llvm::vfs::Status Status;
   if (getStatValue(VF.getName(), Status, /*isFile=*/true, /*F=*/nullptr))
     return None;
llvm::Optional<FileEntryRef> FileManager::getBypassFile(FileEntryRef VF) {
   FileEntry *BFE = new (FilesAlloc.Allocate()) FileEntry();
   BypassFileEntries.push_back(BFE);
   Insertion.first->second = FileEntryRef::MapValue(*BFE, VF.getDir());
   BFE->LastRef = FileEntryRef(*Insertion.first);
   BFE->Size = Status.getSize();
   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);
 }
 
 bool FileManager::FixupRelativePath(SmallVectorImpl<char> &path) const {
   StringRef pathRef(path.data(), path.size());
 
void FileManager::GetUniqueIDMapping(
 
   // Map file entries
   for (llvm::StringMap<llvm::ErrorOr<FileEntryRef::MapValue>,
                        llvm::BumpPtrAllocator>::const_iterator
            FE = SeenFileEntries.begin(),
            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;
     }
 
   // Map virtual file entries
   for (const auto &VFE : VirtualFileEntries)
     UIDToFiles[VFE->getUID()] = VFE;
 }
 

clang/lib/Frontend/LogDiagnosticPrinter.cpp

void LogDiagnosticPrinter::HandleDiagnostic(DiagnosticsEngine::Level Level,
   // Default implementation (Warnings/errors count).
   DiagnosticConsumer::HandleDiagnostic(Level, Info);
 
   // Initialize the main file name, if we haven't already fetched it.
   if (MainFilename.empty() && Info.hasSourceManager()) {
     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());
     }
   }
 
   // Create the diag entry.
   DiagEntry DE;
   DE.DiagnosticID = Info.getID();
   DE.DiagnosticLevel = Level;
void LogDiagnosticPrinter::HandleDiagnostic(DiagnosticsEngine::Level Level,
   if (Info.getLocation().isValid() && Info.hasSourceManager()) {
     const SourceManager &SM = Info.getSourceManager();
     PresumedLoc PLoc = SM.getPresumedLoc(Info.getLocation());
 
     if (PLoc.isInvalid()) {
       // 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 {
       DE.Filename = PLoc.getFilename();
       DE.Line = PLoc.getLine();
       DE.Column = PLoc.getColumn();
     }
   }
 
   // Record the diagnostic entry.
   Entries.push_back(DE);
 }
 

clang/lib/Frontend/TextDiagnostic.cpp

 /// ranges necessary.
 void TextDiagnostic::emitDiagnosticLoc(FullSourceLoc Loc, PresumedLoc PLoc,
                                        DiagnosticsEngine::Level Level,
                                        ArrayRef<CharSourceRange> Ranges) {
   if (PLoc.isInvalid()) {
     // 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 << ": ";
       }
     }
     return;
   }
   unsigned LineNo = PLoc.getLine();
 

clang/unittests/Basic/FileEntryTest.cpp

 //===- unittests/Basic/FileEntryTest.cpp - Test FileEntry/FileEntryRef ----===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #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;
 using namespace clang;
 
 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(
-        *Dirs.insert({Name, const_cast<DirectoryEntry &>(Base.getDirEntry())})
-             .first);
+    return DirectoryEntryRef(*Dirs.insert({Name, Base.getDirEntry()}).first);
   }
 
   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.try_emplace(Name, FileEntryRef::MapValue(*FE, DR)).first);
   }
   FileEntryRef addFileAlias(StringRef Name, FileEntryRef Base) {
     return FileEntryRef(
         *Files
-             .insert(
-                 {Name, FileEntryRef::MapValue(
-                            const_cast<FileEntry &>(Base.getFileEntry()), DR)})
+             .try_emplace(Name, FileEntryRef::MapValue(Base.getFileEntry(), DR))
              .first);
   }
 };
 
-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");
   FileEntryRef R2 = Refs.addFile("2");
   FileEntryRef R1Also = Refs.addFileAlias("1-also", R1);
 
   EXPECT_EQ("1", R1.getName());
   EXPECT_EQ("2", R2.getName());

clang/unittests/Basic/FileManagerTest.cpp

#ifdef _WIN32
   statCache->InjectDirectory(DirName, 44);
   statCache->InjectFile(FileName, 45);
 #endif
 
   manager.setStatCache(std::move(statCache));
 
   auto file = manager.getFile("/tmp/test");
   ASSERT_TRUE(file);
-  ASSERT_TRUE((*file)->isValid());
   EXPECT_EQ("/tmp/test", (*file)->getName());
 
   const DirectoryEntry *dir = (*file)->getDir();
   ASSERT_TRUE(dir != nullptr);
   EXPECT_EQ("/tmp", dir->getName());
 
 #ifdef _WIN32
   file = manager.getFile(FileName);
   ASSERT_TRUE(file);
 
   dir = (*file)->getDir();
   ASSERT_TRUE(dir != NULL);
   EXPECT_EQ(DirName, dir->getName());
 #endif
 }
 
 // getFile() returns non-NULL if a virtual file exists at the given path.
 TEST_F(FileManagerTest, getFileReturnsValidFileEntryForExistingVirtualFile) {
   // Fake an empty real file system.
   manager.setStatCache(std::make_unique<FakeStatCache>());
 
   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();
   ASSERT_TRUE(dir != nullptr);
   EXPECT_EQ("virtual/dir", dir->getName());
 }
 
 // getFile() returns different FileEntries for different paths when
 // there's no aliasing.
 TEST_F(FileManagerTest, getFileReturnsDifferentFileEntriesForDifferentFiles) {
   // Inject two fake files into the file system.  Different inodes
   // mean the files are not symlinked together.
   auto statCache = std::make_unique<FakeStatCache>();
   statCache->InjectDirectory(".", 41);
   statCache->InjectFile("foo.cpp", 42);
   statCache->InjectFile("bar.cpp", 43);
   manager.setStatCache(std::move(statCache));
 
   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);
 }
 
 // getFile() returns an error if neither a real file nor a virtual file
 // exists at the given path.
 TEST_F(FileManagerTest, getFileReturnsErrorForNonexistentFile) {
   // Inject a fake foo.cpp into the file system.
   auto statCache = std::make_unique<FakeStatCache>();

 TEST_F(FileManagerTest, getFileReturnsSameFileEntryForAliasedVirtualFiles) {
   // Inject two real files with the same inode.
   auto statCache = std::make_unique<FakeStatCache>();
   statCache->InjectDirectory("abc", 41);
   statCache->InjectFile("abc/foo.cpp", 42);
   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");
 
   EXPECT_EQ(f1 ? *f1 : nullptr,
             f2 ? *f2 : nullptr);
 }
 
 TEST_F(FileManagerTest, getFileRefEquality) {
TEST_F(FileManagerTest, getVirtualFileWithDifferentName) {
   statCache->InjectDirectory("c:\\tmp", 42);
   statCache->InjectFile("c:\\tmp\\test", 43);
 
   manager.setStatCache(std::move(statCache));
 
   // 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());
   // Check that the contents of the UFE are not overwritten by the entry in the
   // filesystem:
   EXPECT_EQ(123, (*file2)->getSize());
 }
 
TEST_F(FileManagerTest, getVirtualFileFillsRealPathName) {
   statCache->InjectDirectory("/tmp", 42);
   statCache->InjectFile("/tmp/test", 43);
 
   Manager.setStatCache(std::move(statCache));
 
   // 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");
   EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult);
 }
 
 TEST_F(FileManagerTest, getFileDontOpenRealPath) {
   SmallString<64> CustomWorkingDir = getSystemRoot();
TEST_F(FileManagerTest, getFileDontOpenRealPath) {
   statCache->InjectDirectory("/tmp", 42);
   statCache->InjectFile("/tmp/test", 43);
 
   Manager.setStatCache(std::move(statCache));
 
   // 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");
   EXPECT_EQ((*file)->tryGetRealPathName(), ExpectedResult);
 }
 
 TEST_F(FileManagerTest, getBypassFile) {
   SmallString<64> CustomWorkingDir;
#endif
   Cache->InjectDirectory("/tmp", 42);
   Cache->InjectFile("/tmp/test", 43);
   Manager.setStatCache(std::move(Cache));
 
   // Set up a virtual file with a different size than FakeStatCache uses.
   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.
   unsigned VirtualUID = FE.getUID();
   llvm::Optional<FileEntryRef> SearchRef;
   ASSERT_THAT_ERROR(Manager.getFileRef("/tmp/test").moveInto(SearchRef),
                     Succeeded());
   EXPECT_EQ(&FE, &SearchRef->getFileEntry());
   EXPECT_EQ(FE.getUID(), VirtualUID);
   EXPECT_EQ(FE.getSize(), 10);
 
   // Bypass the file.
   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.
   EXPECT_NE(&BypassRef->getFileEntry(), File);
   EXPECT_NE(BypassRef->getUID(), VirtualUID);
   EXPECT_NE(BypassRef->getSize(), FE.getSize());
 
   // The virtual file should still be returned when searching.
   ASSERT_THAT_ERROR(Manager.getFileRef("/tmp/test").moveInto(SearchRef),
                     Succeeded());
   EXPECT_EQ(&FE, &SearchRef->getFileEntry());
 }
 
 } // anonymous namespace