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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
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___");
----------------
klimek wrote:
> ilya-biryukov wrote:
> > klimek wrote:
> > > ilya-biryukov wrote:
> > > > klimek wrote:
> > > > > 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?
> > > > Yeah, `erasedOnReboot` seems weird here.
> > > > What I really wanted is a filepath that's valid on all platforms and is on an existing drive for Windows, so the value of `erasedOnReboot` doesn't really matter. `system_temp_directory` might be the wrong thing to use for that in the first place. Is there a better alternative I'm missing?
> > > Why does it need to get an existing path / existing drive?
> > Oh, maybe it doesn't have to be. But I expect that a probability of something breaking is lower if we use an existing path.
> > Maybe I'm wrong and it will all work out just fine, but that seems to be a rather good compromise adding an extra bit of confidence that things will be working in the long-term.
> I'd on the contrary say that this is more likely to fail. If we don't have a real filesystem, or have a read-only view of a real file system, getting a temp dir might well fail, while we can easily overlay any path we want in memory.
That's true in general, but LLVM's `system_temp_directory` will return the default (`/var/tmp` or `/tmp` on Linux, `C:\Temp` on Windows) if it fails to find the real temp dir.
So in the very worst case we'll be using hard-coded paths.

On the other hand, if that's one of the possible behaviors, we'd probably want to not break in that case anyway. I'll update the patch to follow your suggestions. Thanks!


https://reviews.llvm.org/D39842