[PATCH] D89488: FileManager: Shrink FileEntryRef to the size of a pointer
dexonsmith added inline comments.
Comment at: clang/include/clang/Basic/FileManager.h:101
+ /// Type used in the StringMap.
+ using MapEntry = llvm::StringMapEntry>;
+
thakis wrote:
> thakis wrote:
> > It looks like this is too clever for gcc5.3:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=1143022
> >
> > Ideas on how to work around that?
> Looks like the following might do it. I'll finish building locally and push
> this if it works:
>
> ```
> $ git diff
> diff --git a/clang/include/clang/Basic/FileManager.h
> b/clang/include/clang/Basic/FileManager.h
> index d27b4260cca..12848f42770 100644
> --- a/clang/include/clang/Basic/FileManager.h
> +++ b/clang/include/clang/Basic/FileManager.h
> @@ -107,7 +107,9 @@ public:
> /// 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.
> -llvm::PointerUnion V;
> +// The second type is really a `const MapEntry *`, but that confuses
> gcc5.3.
> +// Once that's no longer supported, change this back.
> +llvm::PointerUnion V;
>
> MapValue() = delete;
> MapValue(FileEntry ) : V() {}
> diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
> index d26ead4a5b0..4d84c3102ec 100644
> --- a/clang/lib/Basic/FileManager.cpp
> +++ b/clang/lib/Basic/FileManager.cpp
> @@ -215,7 +215,7 @@ FileManager::getFileRef(StringRef Filename, bool
> openFile, bool CacheFailure) {
> FileEntryRef::MapValue Value = *SeenFileInsertResult.first->second;
> if (LLVM_LIKELY(Value.V.is()))
>return FileEntryRef(*SeenFileInsertResult.first);
> -return FileEntryRef(*Value.V.get());
> +return FileEntryRef(*reinterpret_cast *>(Value.V.get()));
>}
>
>// We've not seen this before. Fill it in.
> @@ -347,7 +347,7 @@ FileManager::getVirtualFile(StringRef Filename, off_t
> Size,
> FileEntry *FE;
> if (LLVM_LIKELY(FE = Value.V.dyn_cast()))
>return FE;
> -return (*Value.V.get())
> +return (*reinterpret_cast *>(Value.V.get()))
> .getFileEntry();
>}
>
> ```
Thanks Nico!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89488/new/
https://reviews.llvm.org/D89488
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89488: FileManager: Shrink FileEntryRef to the size of a pointer
thakis added inline comments.
Comment at: clang/include/clang/Basic/FileManager.h:101
+ /// Type used in the StringMap.
+ using MapEntry = llvm::StringMapEntry>;
+
thakis wrote:
> It looks like this is too clever for gcc5.3:
> https://bugs.chromium.org/p/chromium/issues/detail?id=1143022
>
> Ideas on how to work around that?
Looks like the following might do it. I'll finish building locally and push
this if it works:
```
$ git diff
diff --git a/clang/include/clang/Basic/FileManager.h
b/clang/include/clang/Basic/FileManager.h
index d27b4260cca..12848f42770 100644
--- a/clang/include/clang/Basic/FileManager.h
+++ b/clang/include/clang/Basic/FileManager.h
@@ -107,7 +107,9 @@ public:
/// 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.
-llvm::PointerUnion V;
+// The second type is really a `const MapEntry *`, but that confuses
gcc5.3.
+// Once that's no longer supported, change this back.
+llvm::PointerUnion V;
MapValue() = delete;
MapValue(FileEntry ) : V() {}
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index d26ead4a5b0..4d84c3102ec 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -215,7 +215,7 @@ FileManager::getFileRef(StringRef Filename, bool openFile,
bool CacheFailure) {
FileEntryRef::MapValue Value = *SeenFileInsertResult.first->second;
if (LLVM_LIKELY(Value.V.is()))
return FileEntryRef(*SeenFileInsertResult.first);
-return FileEntryRef(*Value.V.get());
+return FileEntryRef(*reinterpret_cast(Value.V.get()));
}
// We've not seen this before. Fill it in.
@@ -347,7 +347,7 @@ FileManager::getVirtualFile(StringRef Filename, off_t Size,
FileEntry *FE;
if (LLVM_LIKELY(FE = Value.V.dyn_cast()))
return FE;
-return (*Value.V.get())
+return (*reinterpret_cast(Value.V.get()))
.getFileEntry();
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89488/new/
https://reviews.llvm.org/D89488
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89488: FileManager: Shrink FileEntryRef to the size of a pointer
thakis added inline comments. Herald added a subscriber: ormris. Comment at: clang/include/clang/Basic/FileManager.h:101 + /// Type used in the StringMap. + using MapEntry = llvm::StringMapEntry>; + It looks like this is too clever for gcc5.3: https://bugs.chromium.org/p/chromium/issues/detail?id=1143022 Ideas on how to work around that? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89488/new/ https://reviews.llvm.org/D89488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89488: FileManager: Shrink FileEntryRef to the size of a pointer
This revision was automatically updated to reflect the committed changes.
dexonsmith marked 2 inline comments as done.
Closed by commit rG917acac960d4: FileManager: Shrink FileEntryRef to the size
of a pointer (authored by dexonsmith).
Herald added a project: clang.
Changed prior to commit:
https://reviews.llvm.org/D89488?vs=300811=301074#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89488/new/
https://reviews.llvm.org/D89488
Files:
clang/include/clang/Basic/FileManager.h
clang/lib/Basic/FileManager.cpp
clang/lib/Basic/SourceManager.cpp
clang/unittests/Basic/FileManagerTest.cpp
Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -285,9 +285,18 @@
StatCache->InjectFile("dir/f1-alias.cpp", 41, "dir/f1.cpp");
StatCache->InjectFile("dir/f2.cpp", 42);
StatCache->InjectFile("dir/f2-alias.cpp", 42, "dir/f2.cpp");
+
+ // This unintuitive rename-the-file-on-stat behaviour supports how the
+ // RedirectingFileSystem VFS layer responds to stats. However, even if you
+ // have two layers, you should only get a single filename back. As such the
+ // following stat cache behaviour is not supported (the correct stat entry
+ // for a double-redirection would be "dir/f1.cpp") and the getFileRef below
+ // should assert.
+ StatCache->InjectFile("dir/f1-alias-alias.cpp", 41, "dir/f1-alias.cpp");
+
manager.setStatCache(std::move(StatCache));
- // With F2, test accessing the non-redirected name first.
+ // With F1, test accessing the non-redirected name first.
auto F1 = manager.getFileRef("dir/f1.cpp");
auto F1Alias = manager.getFileRef("dir/f1-alias.cpp");
auto F1Alias2 = manager.getFileRef("dir/f1-alias.cpp");
@@ -301,6 +310,11 @@
EXPECT_EQ(>getFileEntry(), >getFileEntry());
EXPECT_EQ(>getFileEntry(), >getFileEntry());
+#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
+ EXPECT_DEATH((void)manager.getFileRef("dir/f1-alias-alias.cpp"),
+ "filename redirected to a non-canonical filename?");
+#endif
+
// With F2, test accessing the redirected name first.
auto F2Alias = manager.getFileRef("dir/f2-alias.cpp");
auto F2 = manager.getFileRef("dir/f2.cpp");
@@ -487,29 +501,34 @@
// 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);
- FileEntryRef Ref("/tmp/test", *File);
- EXPECT_TRUE(Ref.isValid());
- EXPECT_EQ(Ref.getSize(), 10);
+ const FileEntry = *File;
+ EXPECT_TRUE(FE.isValid());
+ EXPECT_EQ(FE.getSize(), 10);
// Calling a second time should not affect the UID or size.
- unsigned VirtualUID = Ref.getUID();
- EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
- EXPECT_EQ(Ref.getUID(), VirtualUID);
- EXPECT_EQ(Ref.getSize(), 10);
+ unsigned VirtualUID = FE.getUID();
+ EXPECT_EQ(
+ ,
+ (Manager.getFileRef("/tmp/test"))->getFileEntry());
+ EXPECT_EQ(FE.getUID(), VirtualUID);
+ EXPECT_EQ(FE.getSize(), 10);
// Bypass the file.
- llvm::Optional BypassRef = Manager.getBypassFile(Ref);
+ llvm::Optional BypassRef =
+ Manager.getBypassFile(File->getLastRef());
ASSERT_TRUE(BypassRef);
EXPECT_TRUE(BypassRef->isValid());
- EXPECT_EQ(BypassRef->getName(), Ref.getName());
+ EXPECT_EQ("/tmp/test", BypassRef->getName());
// Check that it's different in the right ways.
EXPECT_NE(>getFileEntry(), File);
EXPECT_NE(BypassRef->getUID(), VirtualUID);
- EXPECT_NE(BypassRef->getSize(), Ref.getSize());
+ EXPECT_NE(BypassRef->getSize(), FE.getSize());
// The virtual file should still be returned when searching.
- EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
+ EXPECT_EQ(
+ ,
+ (Manager.getFileRef("/tmp/test"))->getFileEntry());
}
} // anonymous namespace
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -702,7 +702,7 @@
SourceManager::bypassFileContentsOverride(const FileEntry ) {
assert(isFileOverridden());
llvm::Optional BypassFile =
- FileMgr.getBypassFile(FileEntryRef(File.getName(), File));
+ FileMgr.getBypassFile(File.getLastRef());
// If the file can't be found in the FS, give up.
if (!BypassFile)
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -212,11 +212,10 @@
SeenFileInsertResult.first->second.getError());
// Construct and return and FileEntryRef, unless it's a redirect to another
// filename.
-SeenFileEntryOrRedirect Value = *SeenFileInsertResult.first->second;
-FileEntry *FE;
-if
[PATCH] D89488: FileManager: Shrink FileEntryRef to the size of a pointer
dexonsmith marked 3 inline comments as done. dexonsmith added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:217 FileEntry *FE; -if (LLVM_LIKELY(FE = Value.dyn_cast())) - return FileEntryRef(SeenFileInsertResult.first->first(), *FE); -return getFileRef(*Value.get(), openFile, CacheFailure); +if (LLVM_LIKELY(FE = Value.V.dyn_cast())) + return FileEntryRef(*SeenFileInsertResult.first); arphaman wrote: > Can you use `isa` here instead of `dyn_cast`? Yup, done (comment has strangely detached from the code though...) Comment at: clang/lib/Basic/FileManager.cpp:219 + return FileEntryRef(*SeenFileInsertResult.first); +return FileEntryRef(*Value.V.get()); } dexonsmith wrote: > arphaman wrote: > > It looks like this accounts for one level of redirections, but the previous > > implementation could support multiple levels of redirections. Can multiple > > levels of redirections still be supported here too? > I augmented the test for getFileRef to check for multi-level redirections. It > asserts both before and after this patch. Here is a partial backtrace from > before: > ``` > Assertion failed: (is() && "Invalid accessor called"), function get, file > llvm/include/llvm/ADT/PointerUnion.h, line 188. > [...] > 8 BasicTests 0x000103fb5ce9 clang::FileEntry* > llvm::PointerUnion const*>::get() const + 105 > 9 BasicTests 0x000103fb4f80 > clang::FileManager::getFileRef(llvm::StringRef, bool, bool) + 2240 > 10 BasicTests 0x000103d27d57 (anonymous > namespace)::FileManagerTest_getFileRefReturnsCorrectNameForDifferentStatPath_Test::TestBody() > + 711 > ``` > Can you confirm this is the right test to add, or did you mean something > different? We discussed offline and agreed this double-redirection doesn't happen practice. Comment at: clang/lib/Basic/FileManager.cpp:309 // adopt FileEntryRef. -UFE.Name = InterndFileName; - -return FileEntryRef(InterndFileName, UFE); +return *(UFE.LastRef = FileEntryRef(*NamedFileEnt)); } arphaman wrote: > Can you rewrite the FIXME above, and also the assignment return makes it less > readable. Maybe separate out the return onto the next line? Thanks, fixed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89488/new/ https://reviews.llvm.org/D89488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89488: FileManager: Shrink FileEntryRef to the size of a pointer
dexonsmith updated this revision to Diff 300811.
dexonsmith added a comment.
Updated to address remaining review comments.
Note that we discussed double-redirection use case offline, and agreed it
doesn't come up in practice (even with multiple RedirectingFileSystem layers)
so we shouldn't support it. I added a targeted assertion and added an
`EXPECT_DEATH` with a comment in the test.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89488/new/
https://reviews.llvm.org/D89488
Files:
clang/include/clang/Basic/FileManager.h
clang/lib/Basic/FileManager.cpp
clang/lib/Basic/SourceManager.cpp
clang/unittests/Basic/FileManagerTest.cpp
Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -283,9 +283,18 @@
StatCache->InjectFile("dir/f1-alias.cpp", 41, "dir/f1.cpp");
StatCache->InjectFile("dir/f2.cpp", 42);
StatCache->InjectFile("dir/f2-alias.cpp", 42, "dir/f2.cpp");
+
+ // This unintuitive rename-the-file-on-stat behaviour supports how the
+ // RedirectingFileSystem VFS layer responds to stats. However, even if you
+ // have two layers, you should only get a single filename back. As such the
+ // following stat cache behaviour is not supported (the correct stat entry
+ // for a double-redirection would be "dir/f1.cpp") and the getFileRef below
+ // should assert.
+ StatCache->InjectFile("dir/f1-alias-alias.cpp", 41, "dir/f1-alias.cpp");
+
manager.setStatCache(std::move(StatCache));
- // With F2, test accessing the non-redirected name first.
+ // With F1, test accessing the non-redirected name first.
auto F1 = manager.getFileRef("dir/f1.cpp");
auto F1Alias = manager.getFileRef("dir/f1-alias.cpp");
auto F1Alias2 = manager.getFileRef("dir/f1-alias.cpp");
@@ -299,6 +308,11 @@
EXPECT_EQ(>getFileEntry(), >getFileEntry());
EXPECT_EQ(>getFileEntry(), >getFileEntry());
+#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
+ EXPECT_DEATH((void)manager.getFileRef("dir/f1-alias-alias.cpp"),
+ "filename redirected to a non-canonical filename?");
+#endif
+
// With F2, test accessing the redirected name first.
auto F2Alias = manager.getFileRef("dir/f2-alias.cpp");
auto F2 = manager.getFileRef("dir/f2.cpp");
@@ -485,29 +499,34 @@
// 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);
- FileEntryRef Ref("/tmp/test", *File);
- EXPECT_TRUE(Ref.isValid());
- EXPECT_EQ(Ref.getSize(), 10);
+ const FileEntry = *File;
+ EXPECT_TRUE(FE.isValid());
+ EXPECT_EQ(FE.getSize(), 10);
// Calling a second time should not affect the UID or size.
- unsigned VirtualUID = Ref.getUID();
- EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
- EXPECT_EQ(Ref.getUID(), VirtualUID);
- EXPECT_EQ(Ref.getSize(), 10);
+ unsigned VirtualUID = FE.getUID();
+ EXPECT_EQ(
+ ,
+ (Manager.getFileRef("/tmp/test"))->getFileEntry());
+ EXPECT_EQ(FE.getUID(), VirtualUID);
+ EXPECT_EQ(FE.getSize(), 10);
// Bypass the file.
- llvm::Optional BypassRef = Manager.getBypassFile(Ref);
+ llvm::Optional BypassRef =
+ Manager.getBypassFile(File->getLastRef());
ASSERT_TRUE(BypassRef);
EXPECT_TRUE(BypassRef->isValid());
- EXPECT_EQ(BypassRef->getName(), Ref.getName());
+ EXPECT_EQ("/tmp/test", BypassRef->getName());
// Check that it's different in the right ways.
EXPECT_NE(>getFileEntry(), File);
EXPECT_NE(BypassRef->getUID(), VirtualUID);
- EXPECT_NE(BypassRef->getSize(), Ref.getSize());
+ EXPECT_NE(BypassRef->getSize(), FE.getSize());
// The virtual file should still be returned when searching.
- EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
+ EXPECT_EQ(
+ ,
+ (Manager.getFileRef("/tmp/test"))->getFileEntry());
}
} // anonymous namespace
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -707,7 +707,7 @@
SourceManager::bypassFileContentsOverride(const FileEntry ) {
assert(isFileOverridden());
llvm::Optional BypassFile =
- FileMgr.getBypassFile(FileEntryRef(File.getName(), File));
+ FileMgr.getBypassFile(File.getLastRef());
// If the file can't be found in the FS, give up.
if (!BypassFile)
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -212,11 +212,10 @@
SeenFileInsertResult.first->second.getError());
// Construct and return and FileEntryRef, unless it's a redirect to another
// filename.
-SeenFileEntryOrRedirect Value = *SeenFileInsertResult.first->second;
-FileEntry *FE;
-if
[PATCH] D89488: FileManager: Shrink FileEntryRef to the size of a pointer
dexonsmith planned changes to this revision.
dexonsmith added inline comments.
Comment at: clang/lib/Basic/FileManager.cpp:219
+ return FileEntryRef(*SeenFileInsertResult.first);
+return FileEntryRef(*Value.V.get());
}
arphaman wrote:
> It looks like this accounts for one level of redirections, but the previous
> implementation could support multiple levels of redirections. Can multiple
> levels of redirections still be supported here too?
I augmented the test for getFileRef to check for multi-level redirections. It
asserts both before and after this patch. Here is a partial backtrace from
before:
```
Assertion failed: (is() && "Invalid accessor called"), function get, file
llvm/include/llvm/ADT/PointerUnion.h, line 188.
[...]
8 BasicTests 0x000103fb5ce9 clang::FileEntry*
llvm::PointerUnion::get() const + 105
9 BasicTests 0x000103fb4f80
clang::FileManager::getFileRef(llvm::StringRef, bool, bool) + 2240
10 BasicTests 0x000103d27d57 (anonymous
namespace)::FileManagerTest_getFileRefReturnsCorrectNameForDifferentStatPath_Test::TestBody()
+ 711
```
Can you confirm this is the right test to add, or did you mean something
different?
Comment at: clang/unittests/Basic/FileManagerTest.cpp:294
auto F1Alias2 = manager.getFileRef("dir/f1-alias.cpp");
+ auto F1AliasAlias = manager.getFileRef("dir/f1-alias-alias.cpp");
ASSERT_FALSE(!F1);
@arphaman, this is the line the test asserts on (with or without my patch).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89488/new/
https://reviews.llvm.org/D89488
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89488: FileManager: Shrink FileEntryRef to the size of a pointer
dexonsmith updated this revision to Diff 300791.
dexonsmith added a comment.
Adding a test for multi-level indirection.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89488/new/
https://reviews.llvm.org/D89488
Files:
clang/include/clang/Basic/FileManager.h
clang/lib/Basic/FileManager.cpp
clang/lib/Basic/SourceManager.cpp
clang/unittests/Basic/FileManagerTest.cpp
Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -281,37 +281,47 @@
StatCache->InjectDirectory("dir", 40);
StatCache->InjectFile("dir/f1.cpp", 41);
StatCache->InjectFile("dir/f1-alias.cpp", 41, "dir/f1.cpp");
+ StatCache->InjectFile("dir/f1-alias-alias.cpp", 41, "dir/f1-alias.cpp");
StatCache->InjectFile("dir/f2.cpp", 42);
StatCache->InjectFile("dir/f2-alias.cpp", 42, "dir/f2.cpp");
+ StatCache->InjectFile("dir/f2-alias-alias.cpp", 42, "dir/f2-alias.cpp");
manager.setStatCache(std::move(StatCache));
- // With F2, test accessing the non-redirected name first.
+ // With F1, test accessing the non-redirected name first.
auto F1 = manager.getFileRef("dir/f1.cpp");
auto F1Alias = manager.getFileRef("dir/f1-alias.cpp");
auto F1Alias2 = manager.getFileRef("dir/f1-alias.cpp");
+ auto F1AliasAlias = manager.getFileRef("dir/f1-alias-alias.cpp");
ASSERT_FALSE(!F1);
ASSERT_FALSE(!F1Alias);
ASSERT_FALSE(!F1Alias2);
+ ASSERT_FALSE(!F1AliasAlias);
EXPECT_EQ("dir/f1.cpp", F1->getName());
EXPECT_EQ("dir/f1.cpp", F1->getFileEntry().getName());
EXPECT_EQ("dir/f1.cpp", F1Alias->getName());
EXPECT_EQ("dir/f1.cpp", F1Alias2->getName());
+ EXPECT_EQ("dir/f1.cpp", F1AliasAlias->getName());
EXPECT_EQ(>getFileEntry(), >getFileEntry());
EXPECT_EQ(>getFileEntry(), >getFileEntry());
+ EXPECT_EQ(>getFileEntry(), >getFileEntry());
// With F2, test accessing the redirected name first.
+ auto F2AliasAlias = manager.getFileRef("dir/f2-alias-alias.cpp");
auto F2Alias = manager.getFileRef("dir/f2-alias.cpp");
auto F2 = manager.getFileRef("dir/f2.cpp");
auto F2Alias2 = manager.getFileRef("dir/f2-alias.cpp");
ASSERT_FALSE(!F2);
ASSERT_FALSE(!F2Alias);
ASSERT_FALSE(!F2Alias2);
+ ASSERT_FALSE(!F2AliasAlias);
EXPECT_EQ("dir/f2.cpp", F2->getName());
EXPECT_EQ("dir/f2.cpp", F2->getFileEntry().getName());
EXPECT_EQ("dir/f2.cpp", F2Alias->getName());
EXPECT_EQ("dir/f2.cpp", F2Alias2->getName());
+ EXPECT_EQ("dir/f2.cpp", F2AliasAlias->getName());
EXPECT_EQ(>getFileEntry(), >getFileEntry());
EXPECT_EQ(>getFileEntry(), >getFileEntry());
+ EXPECT_EQ(>getFileEntry(), >getFileEntry());
}
// getFile() returns the same FileEntry for virtual files that have
@@ -485,29 +495,34 @@
// 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);
- FileEntryRef Ref("/tmp/test", *File);
- EXPECT_TRUE(Ref.isValid());
- EXPECT_EQ(Ref.getSize(), 10);
+ const FileEntry = *File;
+ EXPECT_TRUE(FE.isValid());
+ EXPECT_EQ(FE.getSize(), 10);
// Calling a second time should not affect the UID or size.
- unsigned VirtualUID = Ref.getUID();
- EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
- EXPECT_EQ(Ref.getUID(), VirtualUID);
- EXPECT_EQ(Ref.getSize(), 10);
+ unsigned VirtualUID = FE.getUID();
+ EXPECT_EQ(
+ ,
+ (Manager.getFileRef("/tmp/test"))->getFileEntry());
+ EXPECT_EQ(FE.getUID(), VirtualUID);
+ EXPECT_EQ(FE.getSize(), 10);
// Bypass the file.
- llvm::Optional BypassRef = Manager.getBypassFile(Ref);
+ llvm::Optional BypassRef =
+ Manager.getBypassFile(File->getLastRef());
ASSERT_TRUE(BypassRef);
EXPECT_TRUE(BypassRef->isValid());
- EXPECT_EQ(BypassRef->getName(), Ref.getName());
+ EXPECT_EQ("/tmp/test", BypassRef->getName());
// Check that it's different in the right ways.
EXPECT_NE(>getFileEntry(), File);
EXPECT_NE(BypassRef->getUID(), VirtualUID);
- EXPECT_NE(BypassRef->getSize(), Ref.getSize());
+ EXPECT_NE(BypassRef->getSize(), FE.getSize());
// The virtual file should still be returned when searching.
- EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
+ EXPECT_EQ(
+ ,
+ (Manager.getFileRef("/tmp/test"))->getFileEntry());
}
} // anonymous namespace
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -707,7 +707,7 @@
SourceManager::bypassFileContentsOverride(const FileEntry ) {
assert(isFileOverridden());
llvm::Optional BypassFile =
- FileMgr.getBypassFile(FileEntryRef(File.getName(), File));
+ FileMgr.getBypassFile(File.getLastRef());
// If the file can't be found in the FS, give up.
if (!BypassFile)
Index:
[PATCH] D89488: FileManager: Shrink FileEntryRef to the size of a pointer
arphaman added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:217 FileEntry *FE; -if (LLVM_LIKELY(FE = Value.dyn_cast())) - return FileEntryRef(SeenFileInsertResult.first->first(), *FE); -return getFileRef(*Value.get(), openFile, CacheFailure); +if (LLVM_LIKELY(FE = Value.V.dyn_cast())) + return FileEntryRef(*SeenFileInsertResult.first); Can you use `isa` here instead of `dyn_cast`? Comment at: clang/lib/Basic/FileManager.cpp:219 + return FileEntryRef(*SeenFileInsertResult.first); +return FileEntryRef(*Value.V.get()); } It looks like this accounts for one level of redirections, but the previous implementation could support multiple levels of redirections. Can multiple levels of redirections still be supported here too? Comment at: clang/lib/Basic/FileManager.cpp:309 // adopt FileEntryRef. -UFE.Name = InterndFileName; - -return FileEntryRef(InterndFileName, UFE); +return *(UFE.LastRef = FileEntryRef(*NamedFileEnt)); } Can you rewrite the FIXME above, and also the assignment return makes it less readable. Maybe separate out the return onto the next line? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89488/new/ https://reviews.llvm.org/D89488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D89488: FileManager: Shrink FileEntryRef to the size of a pointer
dexonsmith updated this revision to Diff 299246.
dexonsmith edited the summary of this revision.
dexonsmith added a comment.
Improved the naming of the helper types for `FileEntryRef` and the
`SeenFileEntries` map and removed dead code for the old typedef. Found when
working on a follow-up.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89488/new/
https://reviews.llvm.org/D89488
Files:
clang/include/clang/Basic/FileManager.h
clang/lib/Basic/FileManager.cpp
clang/lib/Basic/SourceManager.cpp
clang/unittests/Basic/FileManagerTest.cpp
Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -485,29 +485,34 @@
// 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);
- FileEntryRef Ref("/tmp/test", *File);
- EXPECT_TRUE(Ref.isValid());
- EXPECT_EQ(Ref.getSize(), 10);
+ const FileEntry = *File;
+ EXPECT_TRUE(FE.isValid());
+ EXPECT_EQ(FE.getSize(), 10);
// Calling a second time should not affect the UID or size.
- unsigned VirtualUID = Ref.getUID();
- EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
- EXPECT_EQ(Ref.getUID(), VirtualUID);
- EXPECT_EQ(Ref.getSize(), 10);
+ unsigned VirtualUID = FE.getUID();
+ EXPECT_EQ(
+ ,
+ (Manager.getFileRef("/tmp/test"))->getFileEntry());
+ EXPECT_EQ(FE.getUID(), VirtualUID);
+ EXPECT_EQ(FE.getSize(), 10);
// Bypass the file.
- llvm::Optional BypassRef = Manager.getBypassFile(Ref);
+ llvm::Optional BypassRef =
+ Manager.getBypassFile(File->getLastRef());
ASSERT_TRUE(BypassRef);
EXPECT_TRUE(BypassRef->isValid());
- EXPECT_EQ(BypassRef->getName(), Ref.getName());
+ EXPECT_EQ("/tmp/test", BypassRef->getName());
// Check that it's different in the right ways.
EXPECT_NE(>getFileEntry(), File);
EXPECT_NE(BypassRef->getUID(), VirtualUID);
- EXPECT_NE(BypassRef->getSize(), Ref.getSize());
+ EXPECT_NE(BypassRef->getSize(), FE.getSize());
// The virtual file should still be returned when searching.
- EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
+ EXPECT_EQ(
+ ,
+ (Manager.getFileRef("/tmp/test"))->getFileEntry());
}
} // anonymous namespace
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -707,7 +707,7 @@
SourceManager::bypassFileContentsOverride(const FileEntry ) {
assert(isFileOverridden());
llvm::Optional BypassFile =
- FileMgr.getBypassFile(FileEntryRef(File.getName(), File));
+ FileMgr.getBypassFile(File.getLastRef());
// If the file can't be found in the FS, give up.
if (!BypassFile)
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -212,11 +212,11 @@
SeenFileInsertResult.first->second.getError());
// Construct and return and FileEntryRef, unless it's a redirect to another
// filename.
-SeenFileEntryOrRedirect Value = *SeenFileInsertResult.first->second;
+FileEntryRef::MapValue Value = *SeenFileInsertResult.first->second;
FileEntry *FE;
-if (LLVM_LIKELY(FE = Value.dyn_cast()))
- return FileEntryRef(SeenFileInsertResult.first->first(), *FE);
-return getFileRef(*Value.get(), openFile, CacheFailure);
+if (LLVM_LIKELY(FE = Value.V.dyn_cast()))
+ return FileEntryRef(*SeenFileInsertResult.first);
+return FileEntryRef(*Value.V.get());
}
// We've not seen this before. Fill it in.
@@ -268,24 +268,24 @@
// This occurs when one dir is symlinked to another, for example.
FileEntry = UniqueRealFiles[Status.getUniqueID()];
- NamedFileEnt->second =
-
- // If the name returned by getStatValue is different than Filename, re-intern
- // the name.
- if (Status.getName() != Filename) {
-auto =
-*SeenFileEntries.insert({Status.getName(), }).first;
-assert((*NewNamedFileEnt.second).get() == &&
+ if (Status.getName() == Filename) {
+// The name matches. Set the FileEntry.
+NamedFileEnt->second = FileEntryRef::MapValue(UFE);
+ } else {
+// Name mismatch. We need a redirect. First grab the actual entry we want
+// to return.
+auto =
+*SeenFileEntries.insert({Status.getName(), FileEntryRef::MapValue(UFE)})
+ .first;
+assert(Redirection.second->V.get() == &&
"filename from getStatValue() refers to wrong file");
-InterndFileName = NewNamedFileEnt.first().data();
-// In addition to re-interning the name, construct a redirecting seen file
-// entry, that will point to the name the filesystem actually wants to use.
-StringRef
[PATCH] D89488: FileManager: Shrink FileEntryRef to the size of a pointer
dexonsmith updated this revision to Diff 298530.
dexonsmith retitled this revision from "WIP: FileManager: Shrink FileEntryRef
to the size of a pointer" to "FileManager: Shrink FileEntryRef to the size of a
pointer".
dexonsmith added a comment.
Rebase on top of https://reviews.llvm.org/D89521 to further reduce noise. I
think this is ready to go.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89488/new/
https://reviews.llvm.org/D89488
Files:
clang/include/clang/Basic/FileManager.h
clang/lib/Basic/FileManager.cpp
clang/lib/Basic/SourceManager.cpp
clang/unittests/Basic/FileManagerTest.cpp
Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -485,29 +485,34 @@
// 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);
- FileEntryRef Ref("/tmp/test", *File);
- EXPECT_TRUE(Ref.isValid());
- EXPECT_EQ(Ref.getSize(), 10);
+ const FileEntry = *File;
+ EXPECT_TRUE(FE.isValid());
+ EXPECT_EQ(FE.getSize(), 10);
// Calling a second time should not affect the UID or size.
- unsigned VirtualUID = Ref.getUID();
- EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
- EXPECT_EQ(Ref.getUID(), VirtualUID);
- EXPECT_EQ(Ref.getSize(), 10);
+ unsigned VirtualUID = FE.getUID();
+ EXPECT_EQ(
+ ,
+ (Manager.getFileRef("/tmp/test"))->getFileEntry());
+ EXPECT_EQ(FE.getUID(), VirtualUID);
+ EXPECT_EQ(FE.getSize(), 10);
// Bypass the file.
- llvm::Optional BypassRef = Manager.getBypassFile(Ref);
+ llvm::Optional BypassRef =
+ Manager.getBypassFile(File->getLastRef());
ASSERT_TRUE(BypassRef);
EXPECT_TRUE(BypassRef->isValid());
- EXPECT_EQ(BypassRef->getName(), Ref.getName());
+ EXPECT_EQ("/tmp/test", BypassRef->getName());
// Check that it's different in the right ways.
EXPECT_NE(>getFileEntry(), File);
EXPECT_NE(BypassRef->getUID(), VirtualUID);
- EXPECT_NE(BypassRef->getSize(), Ref.getSize());
+ EXPECT_NE(BypassRef->getSize(), FE.getSize());
// The virtual file should still be returned when searching.
- EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
+ EXPECT_EQ(
+ ,
+ (Manager.getFileRef("/tmp/test"))->getFileEntry());
}
} // anonymous namespace
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -707,7 +707,7 @@
SourceManager::bypassFileContentsOverride(const FileEntry ) {
assert(isFileOverridden());
llvm::Optional BypassFile =
- FileMgr.getBypassFile(FileEntryRef(File.getName(), File));
+ FileMgr.getBypassFile(File.getLastRef());
// If the file can't be found in the FS, give up.
if (!BypassFile)
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -212,11 +212,11 @@
SeenFileInsertResult.first->second.getError());
// Construct and return and FileEntryRef, unless it's a redirect to another
// filename.
-SeenFileEntryOrRedirect Value = *SeenFileInsertResult.first->second;
+SeenFileTableValue Value = *SeenFileInsertResult.first->second;
FileEntry *FE;
-if (LLVM_LIKELY(FE = Value.dyn_cast()))
- return FileEntryRef(SeenFileInsertResult.first->first(), *FE);
-return getFileRef(*Value.get(), openFile, CacheFailure);
+if (LLVM_LIKELY(FE = Value.V.dyn_cast()))
+ return FileEntryRef(*SeenFileInsertResult.first);
+return FileEntryRef(*Value.V.get());
}
// We've not seen this before. Fill it in.
@@ -268,24 +268,24 @@
// This occurs when one dir is symlinked to another, for example.
FileEntry = UniqueRealFiles[Status.getUniqueID()];
- NamedFileEnt->second =
-
- // If the name returned by getStatValue is different than Filename, re-intern
- // the name.
- if (Status.getName() != Filename) {
-auto =
-*SeenFileEntries.insert({Status.getName(), }).first;
-assert((*NewNamedFileEnt.second).get() == &&
+ if (Status.getName() == Filename) {
+// The name matches. Set the FileEntry.
+NamedFileEnt->second = SeenFileTableValue(UFE);
+ } else {
+// Name mismatch. We need a redirect. First grab the actual entry we want
+// to return.
+auto =
+*SeenFileEntries.insert({Status.getName(), SeenFileTableValue(UFE)})
+ .first;
+assert(Redirection.second->V.get() == &&
"filename from getStatValue() refers to wrong file");
-InterndFileName = NewNamedFileEnt.first().data();
-// In addition to re-interning the name, construct a redirecting seen file
-// entry, that will point to the name the
