dexonsmith added a comment.
> The ugly part here:
>
> - FileEntryTest was constructing dummy FileEntrys to wrap in FileEntryRef
> - I changed this to use a FileManager as a factory
It's not clear *why* you changed this to use a FileManager as a factory. It
seems unrelated to removing `FileEntry::isValid()` (but maybe I'm missing
something obvious). It also doesn't seem like an improvement for the test,
since FileManager is full of hacks and twisted logic that's hard to reason
about, and it's useful to test FileEntry / FileEntryRef / etc. in isolation of
that.
Depending on the goal, maybe instead we could abstract out some lower-level
API, FileManagerBase or FileEntryFactory or something, which both FileEntryTest
and FileManager use.
================
Comment at: clang/unittests/Basic/FileEntryTest.cpp:47-52
FileEntryRef addFile(StringRef Name) {
- FEs.push_back(std::make_unique<FileEntry>());
+ const FileEntry *FE =
+ FM.getVirtualFile(std::to_string(UniqueNameCounter++), 0, 0);
return FileEntryRef(
- *Files.insert({Name, FileEntryRef::MapValue(*FEs.back().get(), DR)})
- .first);
+ *Files.try_emplace(Name, FileEntryRef::MapValue(*FE, DR)).first);
}
----------------
I don't love this from a layering perspective. FileEntryTest is testing the
low-level pieces that are used inside FileManager... using FileManager to
generate the FileEntries is awkward.
Maybe it'd be okay if `FileManager::getVirtualFile()` were trivial, but it's
not.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123197/new/
https://reviews.llvm.org/D123197
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
