[PATCH] D123197: Remove a few effectively-unused FileEntry APIs. NFC

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

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