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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: include/clang/Frontend/PrecompiledPreamble.h:124
+/// CanReusePreamble + AddImplicitPreamble to make use of it.
+class PrecompiledPreamble {
+public:
----------------
klimek wrote:
> 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.
Made both inner classes of PrecompiledPreamble instead.
The only thing we have outside PrecompiledPreamble now is PreambleBounds. But it doesn't look like a big distraction.


================
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,
----------------
klimek wrote:
> 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.
Made all three useful functions members(BuildPreamble is now a static member, too, called PrecompiledPreamble::Build).

Also removed some accessors that weren't used outside those functions.
And made a PrecompiledPreamble constructor private, so Build is the only function in the interface that creates a preamble. 


https://reviews.llvm.org/D34287