[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: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 {
----------------
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?


================
Comment at: include/clang/Frontend/PrecompiledPreamble.h:97
+  /// All files have size set.
+  off_t Size;
+
----------------
I'd initialize these.


================
Comment at: include/clang/Frontend/PrecompiledPreamble.h:124
+/// CanReusePreamble + AddImplicitPreamble to make use of it.
+class PrecompiledPreamble {
+public:
----------------
If a user doesn't care about the things above this class, can we move those into an extra header?


================
Comment at: include/clang/Frontend/PrecompiledPreamble.h:142
+private:
+  /// Manages a lifetime of temporary file that stores PCH.
+  TempPCHFile PCHFile;
----------------
the lifetime ... stores a PCH


================
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,
----------------
Why not make it a member instead?


================
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 {
----------------
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.


================
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) {
----------------
If this indeed needs to return a possibly owned buffer, explain when exactly it is owned and when it is unowned.


https://reviews.llvm.org/D34287