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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
