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