Author: Duncan P. N. Exon Smith Date: 2020-10-30T14:06:55-04:00 New Revision: 940d0a310dca31ae97080b068cef92eadfee6367
URL: https://github.com/llvm/llvm-project/commit/940d0a310dca31ae97080b068cef92eadfee6367 DIFF: https://github.com/llvm/llvm-project/commit/940d0a310dca31ae97080b068cef92eadfee6367.diff LOG: Revert "FileManager: Improve the FileEntryRef API and customize its OptionalStorage" and follow-ups This reverts commit 5530fb586f30da9dcb434f6be39198dbf016b866. This reverts commit 010238a296e61cbf6f4d7f4383e26cf00c4e4992. This reverts commit 84e8257937ec6a332aa0b688f4dce57016516ffd. Having trouble getting the bots compiling. Will try again later. Added: Modified: clang/include/clang/Basic/FileEntry.h clang/unittests/Basic/CMakeLists.txt clang/unittests/Basic/FileManagerTest.cpp Removed: clang/unittests/Basic/FileEntryTest.cpp ################################################################################ diff --git a/clang/include/clang/Basic/FileEntry.h b/clang/include/clang/Basic/FileEntry.h index 89788b1e68f9..65149569bb59 100644 --- a/clang/include/clang/Basic/FileEntry.h +++ b/clang/include/clang/Basic/FileEntry.h @@ -31,22 +31,6 @@ class File; 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; @@ -54,9 +38,9 @@ class FileEntry; /// accessed by the FileManager's client. class FileEntryRef { public: - StringRef getName() const { return ME->first(); } + const StringRef getName() const { return Entry->first(); } const FileEntry &getFileEntry() const { - return *ME->second->V.get<FileEntry *>(); + return *Entry->second->V.get<FileEntry *>(); } inline bool isValid() const; @@ -65,26 +49,12 @@ class FileEntryRef { 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.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; + return LHS.Entry == RHS.Entry; } 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; @@ -108,191 +78,20 @@ class FileEntryRef { 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. - /// - /// 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 &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) {} - constexpr 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; - -public: - ~OptionalStorage() = default; - constexpr OptionalStorage() noexcept - : MaybeRef(clang::FileEntryRef::optional_none_tag) {} - 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 { + friend class FileManager; -/// 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; + 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"); } - /// 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; - } + const MapEntry *Entry; }; -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). /// @@ -348,6 +147,10 @@ class FileEntry { 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(); } diff --git a/clang/unittests/Basic/CMakeLists.txt b/clang/unittests/Basic/CMakeLists.txt index add190b4ab0f..b7fa259fd1b5 100644 --- a/clang/unittests/Basic/CMakeLists.txt +++ b/clang/unittests/Basic/CMakeLists.txt @@ -5,7 +5,6 @@ set(LLVM_LINK_COMPONENTS add_clang_unittest(BasicTests CharInfoTest.cpp DiagnosticTest.cpp - FileEntryTest.cpp FileManagerTest.cpp LineOffsetMappingTest.cpp SourceManagerTest.cpp diff --git a/clang/unittests/Basic/FileEntryTest.cpp b/clang/unittests/Basic/FileEntryTest.cpp deleted file mode 100644 index ce57d16d9eae..000000000000 --- a/clang/unittests/Basic/FileEntryTest.cpp +++ /dev/null @@ -1,104 +0,0 @@ -//===- 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 diff --git a/clang/unittests/Basic/FileManagerTest.cpp b/clang/unittests/Basic/FileManagerTest.cpp index 0a1f58f3bb90..43680d5cfbed 100644 --- a/clang/unittests/Basic/FileManagerTest.cpp +++ b/clang/unittests/Basic/FileManagerTest.cpp @@ -350,58 +350,6 @@ TEST_F(FileManagerTest, getFileReturnsSameFileEntryForAliasedVirtualFiles) { 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
