Skip to content

Commit ac49500

Browse files
committed
Reapply "FileManager: Improve the FileEntryRef API and customize its OptionalStorage"
This reverts commit 940d0a3, effectively reapplying 84e8257, after working around the compile errors on the bots that I wasn't seeing locally. I removed the `constexpr` from `OptionalStorage<FileEntryRef>` that I had cargo-culted from the generic version, since `FileEntryRef` isn't relevant in `constexpr` contexts anyway. The original commit message follows: Make a few changes to the `FileEntryRef` API in preparation for propagating it enough to remove `FileEntry::getName()`. - Allow `FileEntryRef` to degrade implicitly 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. (Once `FileEntryRef` is everywhere, we should remove this API.) - Change `operator==` to compare the underlying `FileEntry*`, ignoring any difference in the spelling of the filename. There were 0 users of the existing function because it's not useful. In case comparing the exact named reference becomes important, add/test `isSameRef`. - Add `==` comparisons between `FileEntryRef` and `const FileEntry *` (compares the `FileEntry*`). - Customize `OptionalStorage<FileEntryRef>` to be pointer-sized. Add a private constructor that initializes with `nullptr` and specialize `OptionalStorage` to use it. This unblocks updating fields in size-sensitive data structures that currently use `const FileEntry *`. - Add `OptionalFileEntryRefDegradesToFileEntryPtr`, a wrapper around `Optional<FileEntryRef>` that degrades to `const FileEntry*`. This facilitates future incremental patches, like the same operator on `FileEntryRef`. (Once `FileEntryRef` is everywhere, we should remove this class.) - Remove the unncessary `const` from the by-value return of `FileEntryRef::getName`. - Delete the unused function `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. Differential Revision: https://reviews.llvm.org/D89834
1 parent 27324f2 commit ac49500

File tree

4 files changed

+366
-15
lines changed

4 files changed

+366
-15
lines changed

clang/include/clang/Basic/FileEntry.h

Lines changed: 209 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -31,16 +31,32 @@ class File;
3131

3232
namespace clang {
3333

34+
class FileEntryRef;
35+
36+
} // namespace clang
37+
38+
namespace llvm {
39+
namespace optional_detail {
40+
41+
/// Forward declare a template specialization for OptionalStorage.
42+
template <>
43+
class OptionalStorage<clang::FileEntryRef, /*is_trivially_copyable*/ true>;
44+
45+
} // namespace optional_detail
46+
} // namespace llvm
47+
48+
namespace clang {
49+
3450
class DirectoryEntry;
3551
class FileEntry;
3652

3753
/// A reference to a \c FileEntry that includes the name of the file as it was
3854
/// accessed by the FileManager's client.
3955
class FileEntryRef {
4056
public:
41-
const StringRef getName() const { return Entry->first(); }
57+
StringRef getName() const { return ME->first(); }
4258
const FileEntry &getFileEntry() const {
43-
return *Entry->second->V.get<FileEntry *>();
59+
return *ME->second->V.get<FileEntry *>();
4460
}
4561

4662
inline bool isValid() const;
@@ -49,12 +65,26 @@ class FileEntryRef {
4965
inline const llvm::sys::fs::UniqueID &getUniqueID() const;
5066
inline time_t getModificationTime() const;
5167

68+
/// Check if the underlying FileEntry is the same, intentially ignoring
69+
/// whether the file was referenced with the same spelling of the filename.
5270
friend bool operator==(const FileEntryRef &LHS, const FileEntryRef &RHS) {
53-
return LHS.Entry == RHS.Entry;
71+
return &LHS.getFileEntry() == &RHS.getFileEntry();
72+
}
73+
friend bool operator==(const FileEntry *LHS, const FileEntryRef &RHS) {
74+
return LHS == &RHS.getFileEntry();
75+
}
76+
friend bool operator==(const FileEntryRef &LHS, const FileEntry *RHS) {
77+
return &LHS.getFileEntry() == RHS;
5478
}
5579
friend bool operator!=(const FileEntryRef &LHS, const FileEntryRef &RHS) {
5680
return !(LHS == RHS);
5781
}
82+
friend bool operator!=(const FileEntry *LHS, const FileEntryRef &RHS) {
83+
return !(LHS == RHS);
84+
}
85+
friend bool operator!=(const FileEntryRef &LHS, const FileEntry *RHS) {
86+
return !(LHS == RHS);
87+
}
5888

5989
struct MapValue;
6090

@@ -78,20 +108,188 @@ class FileEntryRef {
78108
MapValue(MapEntry &ME) : V(&ME) {}
79109
};
80110

81-
private:
82-
friend class FileManager;
111+
/// Check if RHS referenced the file in exactly the same way.
112+
bool isSameRef(const FileEntryRef &RHS) const { return ME == RHS.ME; }
113+
114+
/// Allow FileEntryRef to degrade into 'const FileEntry*' to facilitate
115+
/// incremental adoption.
116+
///
117+
/// The goal is to avoid code churn due to dances like the following:
118+
/// \code
119+
/// // Old code.
120+
/// lvalue = rvalue;
121+
///
122+
/// // Temporary code from an incremental patch.
123+
/// lvalue = &rvalue.getFileEntry();
124+
///
125+
/// // Final code.
126+
/// lvalue = rvalue;
127+
/// \endcode
128+
///
129+
/// FIXME: Once FileEntryRef is "everywhere" and FileEntry::LastRef and
130+
/// FileEntry::getName have been deleted, delete this implicit conversion.
131+
operator const FileEntry *() const { return &getFileEntry(); }
83132

84133
FileEntryRef() = delete;
85-
explicit FileEntryRef(const MapEntry &Entry)
86-
: Entry(&Entry) {
87-
assert(Entry.second && "Expected payload");
88-
assert(Entry.second->V && "Expected non-null");
89-
assert(Entry.second->V.is<FileEntry *>() && "Expected FileEntry");
134+
explicit FileEntryRef(const MapEntry &ME) : ME(&ME) {
135+
assert(ME.second && "Expected payload");
136+
assert(ME.second->V && "Expected non-null");
137+
assert(ME.second->V.is<FileEntry *>() && "Expected FileEntry");
90138
}
91139

92-
const MapEntry *Entry;
140+
/// Expose the underlying MapEntry to simplify packing in a PointerIntPair or
141+
/// PointerUnion and allow construction in Optional.
142+
const clang::FileEntryRef::MapEntry &getMapEntry() const { return *ME; }
143+
144+
private:
145+
friend class llvm::optional_detail::OptionalStorage<
146+
FileEntryRef, /*is_trivially_copyable*/ true>;
147+
struct optional_none_tag {};
148+
149+
// Private constructor for use by OptionalStorage.
150+
FileEntryRef(optional_none_tag) : ME(nullptr) {}
151+
bool hasOptionalValue() const { return ME; }
152+
153+
const MapEntry *ME;
93154
};
94155

156+
static_assert(sizeof(FileEntryRef) == sizeof(const FileEntry *),
157+
"FileEntryRef must avoid size overhead");
158+
159+
static_assert(std::is_trivially_copyable<FileEntryRef>::value,
160+
"FileEntryRef must be trivially copyable");
161+
162+
} // end namespace clang
163+
164+
namespace llvm {
165+
namespace optional_detail {
166+
167+
/// Customize OptionalStorage<FileEntryRef> to use FileEntryRef and its
168+
/// optional_none_tag to keep it the size of a single pointer.
169+
template <> class OptionalStorage<clang::FileEntryRef> {
170+
clang::FileEntryRef MaybeRef;
171+
172+
public:
173+
~OptionalStorage() = default;
174+
OptionalStorage() noexcept
175+
: MaybeRef(clang::FileEntryRef::optional_none_tag()) {}
176+
OptionalStorage(OptionalStorage const &Other) = default;
177+
OptionalStorage(OptionalStorage &&Other) = default;
178+
OptionalStorage &operator=(OptionalStorage const &Other) = default;
179+
OptionalStorage &operator=(OptionalStorage &&Other) = default;
180+
181+
template <class... ArgTypes>
182+
explicit OptionalStorage(in_place_t, ArgTypes &&...Args)
183+
: MaybeRef(std::forward<ArgTypes>(Args)...) {}
184+
185+
void reset() noexcept { MaybeRef = clang::FileEntryRef::optional_none_tag(); }
186+
187+
bool hasValue() const noexcept { return MaybeRef.hasOptionalValue(); }
188+
189+
clang::FileEntryRef &getValue() LLVM_LVALUE_FUNCTION noexcept {
190+
assert(hasValue());
191+
return MaybeRef;
192+
}
193+
clang::FileEntryRef const &getValue() const LLVM_LVALUE_FUNCTION noexcept {
194+
assert(hasValue());
195+
return MaybeRef;
196+
}
197+
#if LLVM_HAS_RVALUE_REFERENCE_THIS
198+
clang::FileEntryRef &&getValue() &&noexcept {
199+
assert(hasValue());
200+
return std::move(MaybeRef);
201+
}
202+
#endif
203+
204+
template <class... Args> void emplace(Args &&...args) {
205+
MaybeRef = clang::FileEntryRef(std::forward<Args>(args)...);
206+
}
207+
208+
OptionalStorage &operator=(clang::FileEntryRef Ref) {
209+
MaybeRef = Ref;
210+
return *this;
211+
}
212+
};
213+
214+
static_assert(sizeof(Optional<clang::FileEntryRef>) ==
215+
sizeof(clang::FileEntryRef),
216+
"Optional<FileEntryRef> must avoid size overhead");
217+
218+
static_assert(std::is_trivially_copyable<Optional<clang::FileEntryRef>>::value,
219+
"Optional<FileEntryRef> should be trivially copyable");
220+
221+
} // end namespace optional_detail
222+
} // end namespace llvm
223+
224+
namespace clang {
225+
226+
/// Wrapper around Optional<FileEntryRef> that degrades to 'const FileEntry*',
227+
/// facilitating incremental patches to propagate FileEntryRef.
228+
///
229+
/// This class can be used as return value or field where it's convenient for
230+
/// an Optional<FileEntryRef> to degrade to a 'const FileEntry*'. The purpose
231+
/// is to avoid code churn due to dances like the following:
232+
/// \code
233+
/// // Old code.
234+
/// lvalue = rvalue;
235+
///
236+
/// // Temporary code from an incremental patch.
237+
/// Optional<FileEntryRef> MaybeF = rvalue;
238+
/// lvalue = MaybeF ? &MaybeF.getFileEntry() : nullptr;
239+
///
240+
/// // Final code.
241+
/// lvalue = rvalue;
242+
/// \endcode
243+
///
244+
/// FIXME: Once FileEntryRef is "everywhere" and FileEntry::LastRef and
245+
/// FileEntry::getName have been deleted, delete this class and replace
246+
/// instances with Optional<FileEntryRef>.
247+
class OptionalFileEntryRefDegradesToFileEntryPtr
248+
: public Optional<FileEntryRef> {
249+
public:
250+
OptionalFileEntryRefDegradesToFileEntryPtr() noexcept = default;
251+
OptionalFileEntryRefDegradesToFileEntryPtr(
252+
OptionalFileEntryRefDegradesToFileEntryPtr &&) = default;
253+
OptionalFileEntryRefDegradesToFileEntryPtr(
254+
const OptionalFileEntryRefDegradesToFileEntryPtr &) = default;
255+
OptionalFileEntryRefDegradesToFileEntryPtr &
256+
operator=(OptionalFileEntryRefDegradesToFileEntryPtr &&) = default;
257+
OptionalFileEntryRefDegradesToFileEntryPtr &
258+
operator=(const OptionalFileEntryRefDegradesToFileEntryPtr &) = default;
259+
260+
OptionalFileEntryRefDegradesToFileEntryPtr(llvm::NoneType) {}
261+
OptionalFileEntryRefDegradesToFileEntryPtr(FileEntryRef Ref)
262+
: Optional<FileEntryRef>(Ref) {}
263+
OptionalFileEntryRefDegradesToFileEntryPtr(Optional<FileEntryRef> MaybeRef)
264+
: Optional<FileEntryRef>(MaybeRef) {}
265+
266+
OptionalFileEntryRefDegradesToFileEntryPtr &operator=(llvm::NoneType) {
267+
Optional<FileEntryRef>::operator=(None);
268+
return *this;
269+
}
270+
OptionalFileEntryRefDegradesToFileEntryPtr &operator=(FileEntryRef Ref) {
271+
Optional<FileEntryRef>::operator=(Ref);
272+
return *this;
273+
}
274+
OptionalFileEntryRefDegradesToFileEntryPtr &
275+
operator=(Optional<FileEntryRef> MaybeRef) {
276+
Optional<FileEntryRef>::operator=(MaybeRef);
277+
return *this;
278+
}
279+
280+
/// Degrade to 'const FileEntry *' to allow FileEntry::LastRef and
281+
/// FileEntry::getName have been deleted, delete this class and replace
282+
/// instances with Optional<FileEntryRef>
283+
operator const FileEntry *() const {
284+
return hasValue() ? &getValue().getFileEntry() : nullptr;
285+
}
286+
};
287+
288+
static_assert(
289+
std::is_trivially_copyable<
290+
OptionalFileEntryRefDegradesToFileEntryPtr>::value,
291+
"OptionalFileEntryRefDegradesToFileEntryPtr should be trivially copyable");
292+
95293
/// Cached information about one file (either on disk
96294
/// or in the virtual file system).
97295
///
@@ -147,10 +345,6 @@ class FileEntry {
147345
bool isNamedPipe() const { return IsNamedPipe; }
148346

149347
void closeFile() const;
150-
151-
// Only for use in tests to see if deferred opens are happening, rather than
152-
// relying on RealPathName being empty.
153-
bool isOpenForTests() const { return File != nullptr; }
154348
};
155349

156350
bool FileEntryRef::isValid() const { return getFileEntry().isValid(); }

clang/unittests/Basic/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ set(LLVM_LINK_COMPONENTS
55
add_clang_unittest(BasicTests
66
CharInfoTest.cpp
77
DiagnosticTest.cpp
8+
FileEntryTest.cpp
89
FileManagerTest.cpp
910
LineOffsetMappingTest.cpp
1011
SourceManagerTest.cpp
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
//===- unittests/Basic/FileEntryTest.cpp - Test FileEntry/FileEntryRef ----===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#include "clang/Basic/FileEntry.h"
10+
#include "llvm/ADT/StringMap.h"
11+
#include "gtest/gtest.h"
12+
13+
using namespace llvm;
14+
using namespace clang;
15+
16+
namespace {
17+
18+
using MapEntry = FileEntryRef::MapEntry;
19+
using MapValue = FileEntryRef::MapValue;
20+
using MapType = StringMap<llvm::ErrorOr<MapValue>>;
21+
22+
FileEntryRef addRef(MapType &M, StringRef Name, FileEntry &E) {
23+
return FileEntryRef(*M.insert({Name, MapValue(E)}).first);
24+
}
25+
26+
TEST(FileEntryTest, FileEntryRef) {
27+
MapType Refs;
28+
FileEntry E1, E2;
29+
FileEntryRef R1 = addRef(Refs, "1", E1);
30+
FileEntryRef R2 = addRef(Refs, "2", E2);
31+
FileEntryRef R1Also = addRef(Refs, "1-also", E1);
32+
33+
EXPECT_EQ("1", R1.getName());
34+
EXPECT_EQ("2", R2.getName());
35+
EXPECT_EQ("1-also", R1Also.getName());
36+
37+
EXPECT_EQ(&E1, &R1.getFileEntry());
38+
EXPECT_EQ(&E2, &R2.getFileEntry());
39+
EXPECT_EQ(&E1, &R1Also.getFileEntry());
40+
41+
const FileEntry *CE1 = R1;
42+
EXPECT_EQ(CE1, &E1);
43+
}
44+
45+
TEST(FileEntryTest, OptionalFileEntryRefDegradesToFileEntryPtr) {
46+
MapType Refs;
47+
FileEntry E1, E2;
48+
OptionalFileEntryRefDegradesToFileEntryPtr M0;
49+
OptionalFileEntryRefDegradesToFileEntryPtr M1 = addRef(Refs, "1", E1);
50+
OptionalFileEntryRefDegradesToFileEntryPtr M2 = addRef(Refs, "2", E2);
51+
OptionalFileEntryRefDegradesToFileEntryPtr M0Also = None;
52+
OptionalFileEntryRefDegradesToFileEntryPtr M1Also =
53+
addRef(Refs, "1-also", E1);
54+
55+
EXPECT_EQ(M0, M0Also);
56+
EXPECT_EQ(StringRef("1"), M1->getName());
57+
EXPECT_EQ(StringRef("2"), M2->getName());
58+
EXPECT_EQ(StringRef("1-also"), M1Also->getName());
59+
60+
EXPECT_EQ(&E1, &M1->getFileEntry());
61+
EXPECT_EQ(&E2, &M2->getFileEntry());
62+
EXPECT_EQ(&E1, &M1Also->getFileEntry());
63+
64+
const FileEntry *CE1 = M1;
65+
EXPECT_EQ(CE1, &E1);
66+
}
67+
68+
TEST(FileEntryTest, equals) {
69+
MapType Refs;
70+
FileEntry E1, E2;
71+
FileEntryRef R1 = addRef(Refs, "1", E1);
72+
FileEntryRef R2 = addRef(Refs, "2", E2);
73+
FileEntryRef R1Also = addRef(Refs, "1-also", E1);
74+
75+
EXPECT_EQ(R1, &E1);
76+
EXPECT_EQ(&E1, R1);
77+
EXPECT_EQ(R1, R1Also);
78+
EXPECT_NE(R1, &E2);
79+
EXPECT_NE(&E2, R1);
80+
EXPECT_NE(R1, R2);
81+
82+
OptionalFileEntryRefDegradesToFileEntryPtr M0;
83+
OptionalFileEntryRefDegradesToFileEntryPtr M1 = R1;
84+
85+
EXPECT_EQ(M1, &E1);
86+
EXPECT_EQ(&E1, M1);
87+
EXPECT_NE(M1, &E2);
88+
EXPECT_NE(&E2, M1);
89+
}
90+
91+
TEST(FileEntryTest, isSameRef) {
92+
MapType Refs;
93+
FileEntry E1, E2;
94+
FileEntryRef R1 = addRef(Refs, "1", E1);
95+
FileEntryRef R2 = addRef(Refs, "2", E2);
96+
FileEntryRef R1Also = addRef(Refs, "1-also", E1);
97+
98+
EXPECT_TRUE(R1.isSameRef(FileEntryRef(R1)));
99+
EXPECT_TRUE(R1.isSameRef(FileEntryRef(R1.getMapEntry())));
100+
EXPECT_FALSE(R1.isSameRef(R2));
101+
EXPECT_FALSE(R1.isSameRef(R1Also));
102+
}
103+
104+
} // end namespace

0 commit comments

Comments
 (0)