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 {
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
+  /// 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 
+  /// 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 
+  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.


cfe-commits mailing list

Reply via email to