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
