Page MenuHomePhabricator

Moved code hanlding precompiled preamble out of the ASTUnit.
ClosedPublic

Authored by ilya-biryukov on Jun 16 2017, 11:16 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Jun 16 2017, 11:16 AM
arphaman added inline comments.Jun 17 2017, 4:49 PM
include/clang/Frontend/PrecompiledPreamble.h
248 ↗(On Diff #102853)

Is there a missing declaration here or is this a comment for another declaration?

Removed a stray comment (of a previously removed declaration).

ilya-biryukov added inline comments.Jun 19 2017, 1:15 AM
include/clang/Frontend/PrecompiledPreamble.h
248 ↗(On Diff #102853)

Indeed, I forgot to remove a comment after I removed a declaration.
Thanks for spotting that.

ilya-biryukov marked an inline comment as done.Jun 19 2017, 1:15 AM
klimek added inline comments.Jun 19 2017, 1:41 AM
include/clang/Frontend/PrecompiledPreamble.h
42–43 ↗(On Diff #102993)

a) s/PCHTempFile/TempPCHFile/
b) is the unique-ness the only thing that's special? Perhaps UniqueTempFile or somesuch? Seems nothing PCH-specific is here to see
c) why don't we want to support VFS's here?

97 ↗(On Diff #102993)

I'd initialize these.

124 ↗(On Diff #102993)

If a user doesn't care about the things above this class, can we move those into an extra header?

142 ↗(On Diff #102993)

the lifetime ... stores a PCH

157–160 ↗(On Diff #102993)

Why not make it a member instead?

lib/Frontend/ASTUnit.cpp
98 ↗(On Diff #102993)

I'd instead just have both a unique_ptr for memory management and a pointer (for unowned access). Then, if owned, make the raw pointer point to the unique_ptr, if unowned, the unique_ptr stays nullptr.

131–136 ↗(On Diff #102993)

If this indeed needs to return a possibly owned buffer, explain when exactly it is owned and when it is unowned.

ilya-biryukov marked 2 inline comments as done.
  • Added default initializers to PreambleFileHash.
  • Update file comments to address klimek's questions.
ilya-biryukov added inline comments.Jun 19 2017, 8:28 AM
include/clang/Frontend/PrecompiledPreamble.h
42–43 ↗(On Diff #102993)

a) Done.
c) VFS is read-only, it doesn't have a create/delete operations.

b) I would even call this class 'DeleteFileGuard' and leave only createFromCustomPath method to construct it.
The only reason to call it in a PCH-specific manner is to keep it internally as an implementation detail, since I really wanted to discuss if it's doing the right thing.
It also stores a static set of files it created and removes them in static destructor, I am not sure that a good idea in the first place and also wonder what was the original purpose of this code. (It used atexit before, but static destructors and atexit are equivalent).

I assume it would only remove files when someone 'forgets' to call a destructor (i.e. leaks a heap object) of PrecompiledPreamble (ASTUnit in the older version of the code). If that's the case, I would go for removing the static map altogether and clearing up the memory leaks instead. But if there are other cases where it would delete files I'm not aware of, we might want to keep this code. In any case, I don't want this discussion to block this patch, I would rather start it after the patch lands.

124 ↗(On Diff #102993)

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).

157–160 ↗(On Diff #102993)

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?

lib/Frontend/ASTUnit.cpp
98 ↗(On Diff #102993)

It looks like a mess when you return it from a function. (i.e. should we return a pair<unique_ptr<T>, T*> or accept an out parameter unique_ptr<T> &Owner and return T*?) The function itself wasn't there before, it was extracted as a part of this refactoring.

This is essentially the same thing with a few helper methods to make code using it more readable.
Does it feel too clunky?

131–136 ↗(On Diff #102993)

Done. It's owned when read from disk and not owned when taken from CompilerInvocation.PPOptions' file-to-buffer remappings.

klimek added inline comments.Jun 20 2017, 5:46 AM
lib/Frontend/ASTUnit.cpp
131–136 ↗(On Diff #102993)

After some in person discussion, the idea now is to just always copy and return a unique_ptr

Removed PossiblyOwnedBuffer, added an extra copy instead.
This makes the code much simpler.

klimek added inline comments.Jun 20 2017, 7:41 AM
include/clang/Frontend/PrecompiledPreamble.h
124 ↗(On Diff #102993)

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.

157–160 ↗(On Diff #102993)

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 TempPCHFile an inner class of PrecompiledPreamble.
  • Made PreambleFileHash an inner class of PrecompiledPreamble.
  • Changed stanalone functions to members.
  • Removed some member accessors that were no longer used.
ilya-biryukov added inline comments.Jun 20 2017, 9:11 AM
include/clang/Frontend/PrecompiledPreamble.h
124 ↗(On Diff #102993)

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.

157–160 ↗(On Diff #102993)

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.

This revision is now accepted and ready to land.Jun 21 2017, 3:01 AM
This revision was automatically updated to reflect the committed changes.