[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.


================
Comment at: include/clang/Frontend/PrecompiledPreamble.h:124
+/// CanReusePreamble + AddImplicitPreamble to make use of it.
+class PrecompiledPreamble {
+public:
----------------
ilya-biryukov wrote:
> klimek wrote:
> > If a user doesn't care about the things above this class, can we move those into an extra header?
> Do you have any suggestions of where to put it and how to call it?
> I didn't think it's a good idea to put something like 'PreambleFileHash.h' and 'TempPCHFile.h' into 'include/clang/Frontend/'. (Given that they are essential an implementation detail of PrecompiledPreamble, exposing them in public include folders seems like a bad idea).
TempPCHFile looks like something we just want to put into the .cc file and store as a unique_ptr.
PreambleFileHash seems fine as an extra header.


================
Comment at: include/clang/Frontend/PrecompiledPreamble.h:157-160
+  /// We don't expose PCHFile to avoid changing interface when we'll add an
+  /// in-memory PCH, so we declare this function as friend so that it has access
+  /// to PCHFile field.
+  friend void AddImplicitPreamble(CompilerInvocation &CI,
----------------
ilya-biryukov wrote:
> klimek wrote:
> > Why not make it a member instead?
> To keep BuildPreamble, CanReusePreamble and AddImplicitPreamble close to each other.
> I.e. PrecompiledPreamble only stores data used by these functions.
> 
> We could make all of those members, of course. Do you think that would make API better?
Generally, if these are closely coupled to implementation details of PrecompiledPreample, I think that coupling is strong enough to warrant making them members.
On the other hand, making some functions members, and others non-members, and putting them next to each other in the .cc file also would work.


https://reviews.llvm.org/D34287