[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:42-43
+/// destructors.
+/// An assertion will fire if two PCHTempFiles are created with the same name,
+/// so it's not intended to be used outside preamble-handling.
+class TempPCHFile {
----------------
klimek wrote:
> 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?
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.


================
Comment at: include/clang/Frontend/PrecompiledPreamble.h:124
+/// CanReusePreamble + AddImplicitPreamble to make use of it.
+class PrecompiledPreamble {
+public:
----------------
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).


================
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:
> 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?


================
Comment at: lib/Frontend/ASTUnit.cpp:98
 
-static llvm::sys::SmartMutex<false> &getOnDiskMutex() {
-  static llvm::sys::SmartMutex<false> M(/* recursive = */ true);
-  return M;
-}
+  /// A helper class, storing a either a raw pointer, or unique_ptr to an llvm::MemoryBuffer.
+  struct PossiblyOwnedBuffer {
----------------
klimek wrote:
> 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.
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?


================
Comment at: lib/Frontend/ASTUnit.cpp:131-136
+/// \brief Get a source buffer for \p MainFilePath, handling all file-to-file
+/// and file-to-buffer remappings inside \p Invocation.
+static PossiblyOwnedBuffer
+getBufferForFileHandlingRemapping(const CompilerInvocation &Invocation,
+                                  vfs::FileSystem *VFS,
+                                  StringRef FilePath) {
----------------
klimek wrote:
> If this indeed needs to return a possibly owned buffer, explain when exactly it is owned and when it is unowned.
Done. It's owned when read from disk and not owned when taken from CompilerInvocation.PPOptions' file-to-buffer remappings.


https://reviews.llvm.org/D34287