[PATCH] D39842: Allow to store precompiled preambles in memory.

[Manuel Klimek via Phabricator via cfe-commits]

klimek accepted this revision.
klimek added a comment.
This revision is now accepted and ready to land.

LG



================
Comment at: lib/Frontend/PrecompiledPreamble.cpp:490
   PreprocessorOpts.DisablePCHValidation = true;
+  if (Storage.getKind() == PCHStorage::Kind::TempFile) {
+    const TempPCHFile &PCHFile = Storage.asFile();
----------------
ilya-biryukov wrote:
> klimek wrote:
> > ilya-biryukov wrote:
> > > klimek wrote:
> > > > This looks a bit like we should push it into the PCHStorage.
> > > I've extracted a function here to make the code read simpler.
> > > However, I placed it directly into the `PrecompiledPreamble` instead of `PCHStorage` to keep `PCHStorage` responsible for just one thing: managing the `variant`-like union.
> > It being called PCHStorage makes it sound like it handles the abstraction for how the preamble is stored. Given that the variant-like union is basically an interface with an implementation switch, I think all switching on it is also the responsibility of that class. Otherwise we'd need another abstraction on top of it?
> Abstraction on top of it is `PrecompiledPreamble` itself. And `PCHStorage` is just an implementation detail.
> Does that sound reasonable?
That makes more sense now, thx for explaining. It still seems a bit off to me, but I can't come up with a better solution, so LG.


================
Comment at: lib/Frontend/PrecompiledPreamble.cpp:44
+    SmallString<64> Path;
+    llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/true, Path);
+    llvm::sys::path::append(Path, "___clang_inmemory_preamble___");
----------------
erasedOnReboot seems weird for something that we don't want to have on the actual file system. Why do we even want a temp directory?


https://reviews.llvm.org/D39842