dexonsmith updated this revision to Diff 301394.
dexonsmith retitled this revision from "FileManager: Improve the FileEntryRef
API and customize Optional<FileEntryRef>" to "FileManager: Improve the
FileEntryRef API and customize its OptionalStorage".
dexonsmith edited the summary of this revision.
dexonsmith added a comment.
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89834/new/
https://reviews.llvm.org/D89834
Files:
clang/include/clang/Basic/FileEntry.h
clang/unittests/Basic/CMakeLists.txt
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
@@ -350,6 +350,58 @@
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
Index: clang/unittests/Basic/FileEntryTest.cpp
===================================================================
--- /dev/null
+++ clang/unittests/Basic/FileEntryTest.cpp
@@ -0,0 +1,104 @@
+//===- 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
Index: clang/unittests/Basic/CMakeLists.txt
===================================================================
--- clang/unittests/Basic/CMakeLists.txt
+++ clang/unittests/Basic/CMakeLists.txt
@@ -5,6 +5,7 @@
add_clang_unittest(BasicTests
CharInfoTest.cpp
DiagnosticTest.cpp
+ FileEntryTest.cpp
FileManagerTest.cpp
LineOffsetMappingTest.cpp
SourceManagerTest.cpp
Index: clang/include/clang/Basic/FileEntry.h
===================================================================
--- clang/include/clang/Basic/FileEntry.h
+++ clang/include/clang/Basic/FileEntry.h
@@ -31,6 +31,22 @@
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;
@@ -38,9 +54,9 @@
/// 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;
@@ -49,12 +65,26 @@
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;
@@ -75,20 +105,188 @@
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");
+ }
+
+ /// 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)...);
}
- const MapEntry *Entry;
+ 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).
///
@@ -144,10 +342,6 @@
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(); }
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
