[PATCH] D89488: FileManager: Shrink FileEntryRef to the size of a pointer

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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<FileEntry *>()))
-      return FileEntryRef(SeenFileInsertResult.first->first(), *FE);
-    return getFileRef(*Value.get<const StringRef *>(), openFile, CacheFailure);
+    if (LLVM_LIKELY(FE = Value.V.dyn_cast<FileEntry *>()))
+      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<const FileEntryRef::MapEntry *>());
   }
----------------
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<T>() && "Invalid accessor called"), function get, file llvm/include/llvm/ADT/PointerUnion.h, line 188.
> [...]
> 8  BasicTests               0x0000000103fb5ce9 clang::FileEntry* llvm::PointerUnion<clang::FileEntry*, llvm::StringRef const*>::get<clang::FileEntry*>() const + 105
> 9  BasicTests               0x0000000103fb4f80 clang::FileManager::getFileRef(llvm::StringRef, bool, bool) + 2240
> 10 BasicTests               0x0000000103d27d57 (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