klimek 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); ---------------- 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. ================ 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: > > 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? https://reviews.llvm.org/D39842 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits