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