FileManager: Improve the FileEntryRef API and customize its OptionalStorage

[dexonsmith]

Summary

Make a few changes to the FileEntryRef API in preparation for
propagating it enough to remove FileEntry::getName().

• Allow FileEntryRef to degrade to const FileEntry*. This allows functions currently returning const FileEntry * to be updated to return FileEntryRef without requiring all callers to be updated in the same patch. This helps avoid both (a) massive patches where many fields and locals are updated simultaneously and (b) noisy incremental patches where the first patch adds getFileEntry() at call sites and the second patch removes it.
• Change operator== to compare the underlying FileEntry*, ignoring any difference to the spelling of the name. There were 0 users of the existing function because it's not useful. This avoids adding noisy code where getFileEntry() is used pervasively to compare equality. In case comparing the named reference becomes important, add (and test) the API now called isSameRef.
• Allow operator== with const FileEntry * with matching semantics.
• Customize OptionalStorage<FileEntryRef> to be pointer-sized. Add a private constructor that initializes with nullptr and specialize OptionalStorage to use it.
• Add OptionalFileEntryRefDegradesToFileEntryPtr, a wrapper around Optional<FileEntryRef> that degrades to const FileEntry*, for the same reason as adding it o FileEntryRef itself. After FileEntryRef has been propagated instances of this should be replaced with Optional<FileEntryRef>.
• Remove the const from the return of FileEntryRef::getName.
• Delete the unused FileEntry::isOpenForTests.

Note that there are still FileEntry APIs that aren't wrapped and I
plan to deal with these separately / incrementally, as they are needed.

[dexonsmith]

Attaching the correct diff :/.

[dexonsmith]

Note that https://reviews.llvm.org/D89836 is an example of a follow-up patch that leverages these API changes. The original version of that patch was much, much noisier.

[arphaman]

What's wrong with using Optional<FileEntryRef> instead of MaybeFileEntryRef?

[dexonsmith]

In D89834#2356769, @arphaman wrote:

What's wrong with using Optional<FileEntryRef> instead of MaybeFileEntryRef?

Two problems:

1. const FileEntry* is stored in lots of places. I am nervous after the memory regression from FileEntryRef's introduction (https://reviews.llvm.org/D89580) that doubling the size of these fields will matter. I'd rather save it.

2. As mentioned in the description, the "degrade to const FileEntry *" behaviour greatly reduces the code churn for incremental patches.

I realize in retrospect I can fix the first problem by customizing OptionalStorage<FileEntryRef>.

IIRC, the follow-up patches got significantly (2x? 5x? it was a lot) smaller when I fixed the second problem. I'm not sure if it's worth it.

[dexonsmith]

Dropped MaybeFileEntryRef, instead customizing Optional<FileEntryRef>.

[dexonsmith]

In D89834#2357910, @dexonsmith wrote:

Dropped MaybeFileEntryRef, instead customizing Optional<FileEntryRef>.

As @arphaman pointed out offline, we can actually OptionalStorage instead, by adding add a private constructor to FileEntryRef that nullptr-initializes and only OptionalStorage calls.

The reason I think customizing Optional is better is the ability to add operator const FileEntry *. This avoids having to update large swaths of code like this in an incremental patch:

// old code
lvalue = rvalue;
// new code
auto F = rvalue;
lvalue = F ? &F->getFileEntry() : nullptr;

just to revert it back in a later incremental patch once FileEntryRef has made it further.

This was one the primary reasons I originally added MaybeFileEntryRef.

[dexonsmith]

Okay, here's my third attempt:

• Add a private constructor to FileEntryRef that sets the pointer to nullptr.
• Use that in a specialization of OptionalStorage<FileEntryRef>. This makes the size of Optional<FileEntryRef> the same as a pointer.
• Add OptionalFileEntryRefDegradesToFileEntryPtr, which inherits from Optional<FileEntryRef> and degrades to const FileEntry*. Its name explains exactly what its weird behaviour is, it's easy to grep for, and I'll delete it once FileEntry::getName is gone.

I'll update the two follow-up patches in a moment.

[dexonsmith]

clang-format

[arphaman]

This approach seems like a reasonable compromise, thanks! LGTM.

[danielkiss]

clang/include/clang/Basic/FileEntry.h

186

This broke the build, constexpr could be dropped IMHO.
FileEntry.h(186): error C3615: constexpr function 'llvm::optional_detail::OptionalStorage<clang::FileEntryRef,true>::hasValue' cannot result in a constant expression
FileEntry.h(187): note: failure was caused by call of undefined function or one not declared 'constexpr'
FileEntry.h(187): note: see usage of 'clang::FileEntryRef::hasOptionalValue'

[dexonsmith]

clang/include/clang/Basic/FileEntry.h

186

Thanks, I had the same conclusion, I'll drop the constexpr I cargo-culted from the generic version of OptionalStorage.

Note that I already reverted in 940d0a310dca31ae97080b068cef92eadfee6367, if you update you should be able to proceed. (I'm waiting for the bots to fully recover before I push the fixed version.)

[dexonsmith]

Just pushed again in ac49500cd0484e1b2dcf37fa4c0dade6f113c2c9; bots look happier with this version.

[dexonsmith]

In D89834#2365358, @dexonsmith wrote:

Just pushed again in ac49500cd0484e1b2dcf37fa4c0dade6f113c2c9; bots look happier with this version.

Not all of them though; sanitizer-windows is fastest and liked this version better, but I missed until now that ppc64le-lld-multistage-test had a different opinion. I just pushed 814141f9bd0a64bbedae05773972d140f04f654d, which I hope will close this out.

clang/include/clang/Basic/FileEntry.h

 
 class File;
 
 } // namespace vfs
 } // namespace llvm
 
 namespace clang {
 
+class FileEntryRef;
+
+} // namespace clang
+
+namespace llvm {
+namespace optional_detail {
+
+/// Forward declare a template specialization for OptionalStorage.
+template <>
+class OptionalStorage<clang::FileEntryRef, /*is_trivially_copyable*/ true>;
+
+} // namespace optional_detail
+} // namespace llvm
+
+namespace clang {
+
 class DirectoryEntry;
 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:
-  const StringRef getName() const { return Entry->first(); }
+  StringRef getName() const { return ME->first(); }
   const FileEntry &getFileEntry() const {
-    return *Entry->second->V.get<FileEntry *>();
+    return *ME->second->V.get<FileEntry *>();
   }
 
   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;
 
+  /// Check if the underlying FileEntry is the same, intentially ignoring
+  /// whether the file was referenced with the same spelling of the filename.
   friend bool operator==(const FileEntryRef &LHS, const FileEntryRef &RHS) {
-    return LHS.Entry == RHS.Entry;
+    return &LHS.getFileEntry() == &RHS.getFileEntry();
+  }
+  friend bool operator==(const FileEntry *LHS, const FileEntryRef &RHS) {
+    return LHS == &RHS.getFileEntry();
+  }
+  friend bool operator==(const FileEntryRef &LHS, const FileEntry *RHS) {
+    return &LHS.getFileEntry() == RHS;
   }
   friend bool operator!=(const FileEntryRef &LHS, const FileEntryRef &RHS) {
     return !(LHS == RHS);
   }
+  friend bool operator!=(const FileEntry *LHS, const FileEntryRef &RHS) {
+    return !(LHS == RHS);
+  }
+  friend bool operator!=(const FileEntryRef &LHS, const FileEntry *RHS) {
+    return !(LHS == RHS);
+  }
 
   struct MapValue;
 
   /// Type used in the StringMap.
   using MapEntry = llvm::StringMapEntry<llvm::ErrorOr<MapValue>>;
 
   /// Type stored in the StringMap.
   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;
 
     MapValue() = delete;
     MapValue(FileEntry &FE) : V(&FE) {}
     MapValue(MapEntry &ME) : V(&ME) {}
   };
 
-private:
-  friend class FileManager;
+  /// 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.
+  ///
+  /// The goal is to avoid code churn due to dances like the following:
+  /// \code
+  /// // Old code.
+  /// lvalue = rvalue;
+  ///
+  /// // Temporary code from an incremental patch.
+  /// lvalue = &rvalue.getFileEntry();
+  ///
+  /// // Final code.
+  /// lvalue = rvalue;
+  /// \endcode
+  ///
+  /// 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 &Entry)
-      : Entry(&Entry) {
-    assert(Entry.second && "Expected payload");
-    assert(Entry.second->V && "Expected non-null");
-    assert(Entry.second->V.is<FileEntry *>() && "Expected FileEntry");
+  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");
   }
 
-  const MapEntry *Entry;
+  /// 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 llvm::optional_detail::OptionalStorage<
+      FileEntryRef, /*is_trivially_copyable*/ true>;
+  struct optional_none_tag {};
+
+  // Private constructor for use by OptionalStorage.
+  FileEntryRef(optional_none_tag) : ME(nullptr) {}
+  bool hasOptionalValue() const { return ME; }
+
+  const MapEntry *ME;
 };
 
+static_assert(sizeof(FileEntryRef) == sizeof(const FileEntry *),
+              "FileEntryRef must avoid size overhead");
+
+static_assert(std::is_trivially_copyable<FileEntryRef>::value,
+              "FileEntryRef must be trivially copyable");
+
+} // end namespace clang
+
+namespace llvm {
+namespace optional_detail {
+
+/// Customize OptionalStorage<FileEntryRef> to use FileEntryRef and its
+/// optional_none_tag to keep it the size of a single pointer.
+template <> class OptionalStorage<clang::FileEntryRef> {
+  clang::FileEntryRef MaybeRef = clang::FileEntryRef::optional_none_tag{};
+
+public:
+  ~OptionalStorage() = default;
+  constexpr OptionalStorage() noexcept = default;
+  constexpr OptionalStorage(OptionalStorage const &Other) = default;
+  constexpr OptionalStorage(OptionalStorage &&Other) = default;
+  OptionalStorage &operator=(OptionalStorage const &Other) = default;
+  OptionalStorage &operator=(OptionalStorage &&Other) = default;
+
+  template <class... ArgTypes>
+  constexpr explicit OptionalStorage(in_place_t, ArgTypes &&...Args)
+      : MaybeRef(std::forward<ArgTypes>(Args)...) {}
+
+  void reset() noexcept { MaybeRef = clang::FileEntryRef::optional_none_tag(); }
+
+  constexpr bool hasValue() const noexcept {
+    return MaybeRef.hasOptionalValue();
+  }
+
+  clang::FileEntryRef &getValue() LLVM_LVALUE_FUNCTION noexcept {
+    assert(hasValue());
+    return MaybeRef;
+  }
+  constexpr clang::FileEntryRef const &
+  getValue() const LLVM_LVALUE_FUNCTION noexcept {
+    assert(hasValue());
+    return MaybeRef;
+  }
+#if LLVM_HAS_RVALUE_REFERENCE_THIS
+  clang::FileEntryRef &&getValue() &&noexcept {
+    assert(hasValue());
+    return std::move(MaybeRef);
+  }
+#endif
+
+  template <class... Args> void emplace(Args &&...args) {
+    MaybeRef = clang::FileEntryRef(std::forward<Args>(args)...);
+  }
+
+  OptionalStorage &operator=(clang::FileEntryRef Ref) {
+    MaybeRef = Ref;
+    return *this;
+  }
+};
+
+static_assert(sizeof(Optional<clang::FileEntryRef>) ==
+                  sizeof(clang::FileEntryRef),
+              "Optional<FileEntryRef> must avoid size overhead");
+
+static_assert(std::is_trivially_copyable<Optional<clang::FileEntryRef>>::value,
+              "Optional<FileEntryRef> should be trivially copyable");
+
+} // end namespace optional_detail
+} // end namespace llvm
+
+namespace clang {
+
+/// Wrapper around Optional<FileEntryRef> that degrades to 'const FileEntry*',
+/// facilitating incremental patches to propagate FileEntryRef.
+///
+/// This class can be used as return value or field where it's convenient for
+/// an Optional<FileEntryRef> to degrade to a 'const FileEntry*'. The purpose
+/// is to avoid code churn due to dances like the following:
+/// \code
+/// // Old code.
+/// lvalue = rvalue;
+///
+/// // Temporary code from an incremental patch.
+/// Optional<FileEntryRef> MaybeF = rvalue;
+/// lvalue = MaybeF ? &MaybeF.getFileEntry() : nullptr;
+///
+/// // Final code.
+/// lvalue = rvalue;
+/// \endcode
+///
+/// FIXME: Once FileEntryRef is "everywhere" and FileEntry::LastRef and
+/// FileEntry::getName have been deleted, delete this class and replace
+/// instances with Optional<FileEntryRef>.
+class OptionalFileEntryRefDegradesToFileEntryPtr
+    : public Optional<FileEntryRef> {
+public:
+  constexpr OptionalFileEntryRefDegradesToFileEntryPtr() noexcept = default;
+  constexpr OptionalFileEntryRefDegradesToFileEntryPtr(
+      OptionalFileEntryRefDegradesToFileEntryPtr &&) = default;
+  constexpr OptionalFileEntryRefDegradesToFileEntryPtr(
+      const OptionalFileEntryRefDegradesToFileEntryPtr &) = default;
+  OptionalFileEntryRefDegradesToFileEntryPtr &
+  operator=(OptionalFileEntryRefDegradesToFileEntryPtr &&) = default;
+  OptionalFileEntryRefDegradesToFileEntryPtr &
+  operator=(const OptionalFileEntryRefDegradesToFileEntryPtr &) = default;
+
+  OptionalFileEntryRefDegradesToFileEntryPtr(llvm::NoneType) {}
+  OptionalFileEntryRefDegradesToFileEntryPtr(FileEntryRef Ref)
+      : Optional<FileEntryRef>(Ref) {}
+  OptionalFileEntryRefDegradesToFileEntryPtr(Optional<FileEntryRef> MaybeRef)
+      : Optional<FileEntryRef>(MaybeRef) {}
+
+  OptionalFileEntryRefDegradesToFileEntryPtr &operator=(llvm::NoneType) {
+    Optional<FileEntryRef>::operator=(None);
+    return *this;
+  }
+  OptionalFileEntryRefDegradesToFileEntryPtr &operator=(FileEntryRef Ref) {
+    Optional<FileEntryRef>::operator=(Ref);
+    return *this;
+  }
+  OptionalFileEntryRefDegradesToFileEntryPtr &
+  operator=(Optional<FileEntryRef> MaybeRef) {
+    Optional<FileEntryRef>::operator=(MaybeRef);
+    return *this;
+  }
+
+  /// Degrade to 'const FileEntry *' to allow  FileEntry::LastRef and
+  /// FileEntry::getName have been deleted, delete this class and replace
+  /// instances with Optional<FileEntryRef>
+  operator const FileEntry *() const {
+    return hasValue() ? &getValue().getFileEntry() : nullptr;
+  }
+};
+
+static_assert(
+    std::is_trivially_copyable<
+        OptionalFileEntryRefDegradesToFileEntryPtr>::value,
+    "OptionalFileEntryRefDegradesToFileEntryPtr should be trivially copyable");
+
 /// 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;
 
public:
 
   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;
-
-  // Only for use in tests to see if deferred opens are happening, rather than
-  // relying on RealPathName being empty.
-  bool isOpenForTests() const { return File != nullptr; }
 };
 
 bool FileEntryRef::isValid() const { return getFileEntry().isValid(); }
 
 off_t FileEntryRef::getSize() const { return getFileEntry().getSize(); }
 
 unsigned FileEntryRef::getUID() const { return getFileEntry().getUID(); }
 

clang/unittests/Basic/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_unittest(BasicTests
   CharInfoTest.cpp
   DiagnosticTest.cpp
+  FileEntryTest.cpp
   FileManagerTest.cpp
   LineOffsetMappingTest.cpp
   SourceManagerTest.cpp
   )
 
 clang_target_link_libraries(BasicTests
   PRIVATE
   clangAST
   clangBasic
   clangLex
   )

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 "llvm/ADT/StringMap.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+using MapEntry = FileEntryRef::MapEntry;
+using MapValue = FileEntryRef::MapValue;
+using MapType = StringMap<llvm::ErrorOr<MapValue>>;
+
+FileEntryRef addRef(MapType &M, StringRef Name, FileEntry &E) {
+  return FileEntryRef(*M.insert({Name, MapValue(E)}).first);
+}
+
+TEST(FileEntryTest, FileEntryRef) {
+  MapType Refs;
+  FileEntry E1, E2;
+  FileEntryRef R1 = addRef(Refs, "1", E1);
+  FileEntryRef R2 = addRef(Refs, "2", E2);
+  FileEntryRef R1Also = addRef(Refs, "1-also", E1);
+
+  EXPECT_EQ("1", R1.getName());
+  EXPECT_EQ("2", R2.getName());
+  EXPECT_EQ("1-also", R1Also.getName());
+
+  EXPECT_EQ(&E1, &R1.getFileEntry());
+  EXPECT_EQ(&E2, &R2.getFileEntry());
+  EXPECT_EQ(&E1, &R1Also.getFileEntry());
+
+  const FileEntry *CE1 = R1;
+  EXPECT_EQ(CE1, &E1);
+}
+
+TEST(FileEntryTest, OptionalFileEntryRefDegradesToFileEntryPtr) {
+  MapType Refs;
+  FileEntry E1, E2;
+  OptionalFileEntryRefDegradesToFileEntryPtr M0;
+  OptionalFileEntryRefDegradesToFileEntryPtr M1 = addRef(Refs, "1", E1);
+  OptionalFileEntryRefDegradesToFileEntryPtr M2 = addRef(Refs, "2", E2);
+  OptionalFileEntryRefDegradesToFileEntryPtr M0Also = None;
+  OptionalFileEntryRefDegradesToFileEntryPtr M1Also =
+      addRef(Refs, "1-also", E1);
+
+  EXPECT_EQ(M0, M0Also);
+  EXPECT_EQ(StringRef("1"), M1->getName());
+  EXPECT_EQ(StringRef("2"), M2->getName());
+  EXPECT_EQ(StringRef("1-also"), M1Also->getName());
+
+  EXPECT_EQ(&E1, &M1->getFileEntry());
+  EXPECT_EQ(&E2, &M2->getFileEntry());
+  EXPECT_EQ(&E1, &M1Also->getFileEntry());
+
+  const FileEntry *CE1 = M1;
+  EXPECT_EQ(CE1, &E1);
+}
+
+TEST(FileEntryTest, equals) {
+  MapType Refs;
+  FileEntry E1, E2;
+  FileEntryRef R1 = addRef(Refs, "1", E1);
+  FileEntryRef R2 = addRef(Refs, "2", E2);
+  FileEntryRef R1Also = addRef(Refs, "1-also", E1);
+
+  EXPECT_EQ(R1, &E1);
+  EXPECT_EQ(&E1, R1);
+  EXPECT_EQ(R1, R1Also);
+  EXPECT_NE(R1, &E2);
+  EXPECT_NE(&E2, R1);
+  EXPECT_NE(R1, R2);
+
+  OptionalFileEntryRefDegradesToFileEntryPtr M0;
+  OptionalFileEntryRefDegradesToFileEntryPtr M1 = R1;
+
+  EXPECT_EQ(M1, &E1);
+  EXPECT_EQ(&E1, M1);
+  EXPECT_NE(M1, &E2);
+  EXPECT_NE(&E2, M1);
+}
+
+TEST(FileEntryTest, isSameRef) {
+  MapType Refs;
+  FileEntry E1, E2;
+  FileEntryRef R1 = addRef(Refs, "1", E1);
+  FileEntryRef R2 = addRef(Refs, "2", E2);
+  FileEntryRef R1Also = addRef(Refs, "1-also", E1);
+
+  EXPECT_TRUE(R1.isSameRef(FileEntryRef(R1)));
+  EXPECT_TRUE(R1.isSameRef(FileEntryRef(R1.getMapEntry())));
+  EXPECT_FALSE(R1.isSameRef(R2));
+  EXPECT_FALSE(R1.isSameRef(R1Also));
+}
+
+} // end namespace

clang/unittests/Basic/FileManagerTest.cpp

TEST_F(FileManagerTest, getFileReturnsSameFileEntryForAliasedVirtualFiles) {
 
   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) {
+  auto StatCache = std::make_unique<FakeStatCache>();
+  StatCache->InjectDirectory("dir", 40);
+  StatCache->InjectFile("dir/f1.cpp", 41);
+  StatCache->InjectFile("dir/f1-also.cpp", 41);
+  StatCache->InjectFile("dir/f1-redirect.cpp", 41, "dir/f1.cpp");
+  StatCache->InjectFile("dir/f2.cpp", 42);
+  manager.setStatCache(std::move(StatCache));
+
+  auto F1 = manager.getFileRef("dir/f1.cpp");
+  auto F1Again = manager.getFileRef("dir/f1.cpp");
+  auto F1Also = manager.getFileRef("dir/f1-also.cpp");
+  auto F1Redirect = manager.getFileRef("dir/f1-redirect.cpp");
+  auto F2 = manager.getFileRef("dir/f2.cpp");
+
+  // Check Expected<FileEntryRef> for error.
+  ASSERT_FALSE(!F1);
+  ASSERT_FALSE(!F1Also);
+  ASSERT_FALSE(!F1Again);
+  ASSERT_FALSE(!F1Redirect);
+  ASSERT_FALSE(!F2);
+
+  // Check names.
+  EXPECT_EQ("dir/f1.cpp", F1->getName());
+  EXPECT_EQ("dir/f1.cpp", F1Again->getName());
+  EXPECT_EQ("dir/f1-also.cpp", F1Also->getName());
+  EXPECT_EQ("dir/f1.cpp", F1Redirect->getName());
+  EXPECT_EQ("dir/f2.cpp", F2->getName());
+
+  // Compare against FileEntry*.
+  EXPECT_EQ(&F1->getFileEntry(), *F1);
+  EXPECT_EQ(*F1, &F1->getFileEntry());
+  EXPECT_NE(&F2->getFileEntry(), *F1);
+  EXPECT_NE(*F1, &F2->getFileEntry());
+
+  // Compare using ==.
+  EXPECT_EQ(*F1, *F1Also);
+  EXPECT_EQ(*F1, *F1Again);
+  EXPECT_EQ(*F1, *F1Redirect);
+  EXPECT_EQ(*F1Also, *F1Redirect);
+  EXPECT_NE(*F2, *F1);
+  EXPECT_NE(*F2, *F1Also);
+  EXPECT_NE(*F2, *F1Again);
+  EXPECT_NE(*F2, *F1Redirect);
+
+  // Compare using isSameRef.
+  EXPECT_TRUE(F1->isSameRef(*F1Again));
+  EXPECT_TRUE(F1->isSameRef(*F1Redirect));
+  EXPECT_FALSE(F1->isSameRef(*F1Also));
+  EXPECT_FALSE(F1->isSameRef(*F2));
+}
+
 // getFile() Should return the same entry as getVirtualFile if the file actually
 // is a virtual file, even if the name is not exactly the same (but is after
 // normalisation done by the file system, like on Windows). This can be checked
 // here by checking the size.
 TEST_F(FileManagerTest, getVirtualFileWithDifferentName) {
   // Inject fake files into the file system.
   auto statCache = std::make_unique<FakeStatCache>();
   statCache->InjectDirectory("c:\\tmp", 42);