ilya-biryukov added inline comments.
================
Comment at: lib/Frontend/PrecompiledPreamble.cpp:206
+ std::unique_ptr<std::string> Storage;
+ if (InMemStorage) {
+ OS = llvm::make_unique<llvm::raw_string_ostream>(*InMemStorage);
----------------
klimek wrote:
> ilya-biryukov wrote:
> > klimek wrote:
> > > It looks like we should pass in the output stream, not the storage?
> > We're not actually using the `Storage` variable, it's a leftover from
> > previous versions. Removed it.
> >
> > Or did you mean that we should pass in the output stream directly to
> > `PrecompilePreambleAction`'s constructor?
> Yes, I'm generally looking at things that might be better to decide at a
> higher abstraction level and pass in, rather than having switches for
> behavior (like InMemStorage) all over the place. Generally, I think we should
> have a storage (PCHStorage sounds like it was the right abstraction, but
> perhaps that one currently has a bad name) and the things dealing with that
> storage shouldn't care whether it's in memory or on the file system.
It's a bit easier to share code with `GeneratePCHAction` this way in a way that
would obviously works (as we call the same functions at the same points in the
compilation pipeline, that is in `CreateASTConsumer`).
`PCHStorage` is not a public interface and `PrecompiledPreamble` is exactly the
interface that let's you use it without caring whether PCHs are stored in
memory or on disk. It also feels ok for it to have the storage-dependent code
as part of its own implementation.
================
Comment at: lib/Frontend/PrecompiledPreamble.cpp:490
PreprocessorOpts.DisablePCHValidation = true;
+ if (Storage.getKind() == PCHStorage::Kind::TempFile) {
+ const TempPCHFile &PCHFile = Storage.asFile();
----------------
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?
https://reviews.llvm.org/D39842
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
