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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits