[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL305890: Moved code hanlding precompiled preamble out of the
ASTUnit. (authored by ibiryukov).
Changed prior to commit:
https://reviews.llvm.org/D34287?vs=103216=103338#toc
Repository:
rL LLVM
https://reviews.llvm.org/D34287
Files:
cfe/trunk/include/clang/Frontend/ASTUnit.h
cfe/trunk/include/clang/Frontend/PrecompiledPreamble.h
cfe/trunk/lib/Frontend/ASTUnit.cpp
cfe/trunk/lib/Frontend/CMakeLists.txt
cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp
Index: cfe/trunk/include/clang/Frontend/ASTUnit.h
===
--- cfe/trunk/include/clang/Frontend/ASTUnit.h
+++ cfe/trunk/include/clang/Frontend/ASTUnit.h
@@ -25,6 +25,7 @@
#include "clang/Lex/PreprocessingRecord.h"
#include "clang/Sema/CodeCompleteConsumer.h"
#include "clang/Serialization/ASTBitCodes.h"
+#include "clang/Frontend/PrecompiledPreamble.h"
#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringMap.h"
@@ -199,103 +200,15 @@
/// of that loading. It must be cleared when preamble is recreated.
llvm::StringMap PreambleSrcLocCache;
-public:
- class PreambleData {
-const FileEntry *File;
-std::vector Buffer;
-mutable unsigned NumLines;
-
- public:
-PreambleData() : File(nullptr), NumLines(0) { }
-
-void assign(const FileEntry *F, const char *begin, const char *end) {
- File = F;
- Buffer.assign(begin, end);
- NumLines = 0;
-}
-
-void clear() { Buffer.clear(); File = nullptr; NumLines = 0; }
-
-size_t size() const { return Buffer.size(); }
-bool empty() const { return Buffer.empty(); }
-
-const char *getBufferStart() const { return [0]; }
-
-unsigned getNumLines() const {
- if (NumLines)
-return NumLines;
- countLines();
- return NumLines;
-}
-
-SourceRange getSourceRange(const SourceManager ) const {
- SourceLocation FileLoc = SM.getLocForStartOfFile(SM.getPreambleFileID());
- return SourceRange(FileLoc, FileLoc.getLocWithOffset(size()-1));
-}
-
- private:
-void countLines() const;
- };
-
- const PreambleData () const {
-return Preamble;
- }
-
- /// Data used to determine if a file used in the preamble has been changed.
- struct PreambleFileHash {
-/// All files have size set.
-off_t Size;
-
-/// Modification time is set for files that are on disk. For memory
-/// buffers it is zero.
-time_t ModTime;
-
-/// Memory buffers have MD5 instead of modification time. We don't
-/// compute MD5 for on-disk files because we hope that modification time is
-/// enough to tell if the file was changed.
-llvm::MD5::MD5Result MD5;
-
-static PreambleFileHash createForFile(off_t Size, time_t ModTime);
-static PreambleFileHash
-createForMemoryBuffer(const llvm::MemoryBuffer *Buffer);
-
-friend bool operator==(const PreambleFileHash ,
- const PreambleFileHash );
-
-friend bool operator!=(const PreambleFileHash ,
- const PreambleFileHash ) {
- return !(LHS == RHS);
-}
- };
-
private:
- /// \brief The contents of the preamble that has been precompiled to
- /// \c PreambleFile.
- PreambleData Preamble;
-
- /// \brief Whether the preamble ends at the start of a new line.
- ///
- /// Used to inform the lexer as to whether it's starting at the beginning of
- /// a line after skipping the preamble.
- bool PreambleEndsAtStartOfLine;
-
- /// \brief Keeps track of the files that were used when computing the
- /// preamble, with both their buffer size and their modification time.
- ///
- /// If any of the files have changed from one compile to the next,
- /// the preamble must be thrown away.
- llvm::StringMap FilesInPreamble;
+ /// The contents of the preamble.
+ llvm::Optional Preamble;
/// \brief When non-NULL, this is the buffer used to store the contents of
/// the main file when it has been padded for use with the precompiled
/// preamble.
std::unique_ptr SavedMainFileBuffer;
- /// \brief When non-NULL, this is the buffer used to store the
- /// contents of the preamble when it has been padded to build the
- /// precompiled preamble.
- std::unique_ptr PreambleBuffer;
-
/// \brief The number of warnings that occurred while parsing the preamble.
///
/// This value will be used to restore the state of the \c DiagnosticsEngine
@@ -438,21 +351,6 @@
std::unique_ptr OverrideMainBuffer,
IntrusiveRefCntPtr VFS);
- struct ComputedPreamble {
-llvm::MemoryBuffer *Buffer;
-std::unique_ptr Owner;
-unsigned Size;
-bool PreambleEndsAtStartOfLine;
-ComputedPreamble(llvm::MemoryBuffer *Buffer,
- std::unique_ptr Owner, unsigned Size,
- bool PreambleEndsAtStartOfLine)
-: Buffer(Buffer),
[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.
klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg https://reviews.llvm.org/D34287 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.
ilya-biryukov added inline comments.
Comment at: include/clang/Frontend/PrecompiledPreamble.h:124
+/// CanReusePreamble + AddImplicitPreamble to make use of it.
+class PrecompiledPreamble {
+public:
klimek wrote:
> ilya-biryukov wrote:
> > 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).
> TempPCHFile looks like something we just want to put into the .cc file and
> store as a unique_ptr.
> PreambleFileHash seems fine as an extra header.
Made both inner classes of PrecompiledPreamble instead.
The only thing we have outside PrecompiledPreamble now is PreambleBounds. But
it doesn't look like a big distraction.
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 ,
klimek wrote:
> ilya-biryukov wrote:
> > 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?
> Generally, if these are closely coupled to implementation details of
> PrecompiledPreample, I think that coupling is strong enough to warrant making
> them members.
> On the other hand, making some functions members, and others non-members, and
> putting them next to each other in the .cc file also would work.
Made all three useful functions members(BuildPreamble is now a static member,
too, called PrecompiledPreamble::Build).
Also removed some accessors that weren't used outside those functions.
And made a PrecompiledPreamble constructor private, so Build is the only
function in the interface that creates a preamble.
https://reviews.llvm.org/D34287
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.
ilya-biryukov updated this revision to Diff 103216.
ilya-biryukov added a comment.
- Made TempPCHFile an inner class of PrecompiledPreamble.
- Made PreambleFileHash an inner class of PrecompiledPreamble.
- Changed stanalone functions to members.
- Removed some member accessors that were no longer used.
https://reviews.llvm.org/D34287
Files:
include/clang/Frontend/ASTUnit.h
include/clang/Frontend/PrecompiledPreamble.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/CMakeLists.txt
lib/Frontend/PrecompiledPreamble.cpp
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- /dev/null
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -0,0 +1,561 @@
+//===--- PrecompiledPreamble.cpp - Build precompiled preambles --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Helper class to build precompiled preamble.
+//
+//===--===//
+
+#include "clang/Frontend/PrecompiledPreamble.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Serialization/ASTWriter.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/Support/CrashRecoveryContext.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/MutexGuard.h"
+
+using namespace clang;
+
+namespace {
+
+/// Keeps a track of files to be deleted in destructor.
+class TemporaryFiles {
+public:
+ // A static instance to be used by
+ static TemporaryFiles ();
+
+private:
+ // Disallow constructing the class directly.
+ TemporaryFiles() = default;
+ // Disallow copy.
+ TemporaryFiles(const TemporaryFiles &) = delete;
+
+public:
+ ~TemporaryFiles();
+
+ /// Adds \p File to a set of tracked files.
+ void addFile(StringRef File);
+
+ /// Remove \p File from disk and from the set of tracked files.
+ void removeFile(StringRef File);
+
+private:
+ llvm::sys::SmartMutex Mutex;
+ llvm::StringSet<> Files;
+};
+
+TemporaryFiles ::getInstance() {
+ static TemporaryFiles Instance;
+ return Instance;
+}
+
+TemporaryFiles::~TemporaryFiles() {
+ llvm::MutexGuard Guard(Mutex);
+ for (const auto : Files)
+llvm::sys::fs::remove(File.getKey());
+}
+
+void TemporaryFiles::addFile(StringRef File) {
+ llvm::MutexGuard Guard(Mutex);
+ auto IsInserted = Files.insert(File).second;
+ assert(IsInserted && "File has already been added");
+}
+
+void TemporaryFiles::removeFile(StringRef File) {
+ llvm::MutexGuard Guard(Mutex);
+ auto WasPresent = Files.erase(File);
+ assert(WasPresent && "File was not tracked");
+ llvm::sys::fs::remove(File);
+}
+
+class PreambleMacroCallbacks : public PPCallbacks {
+public:
+ PreambleMacroCallbacks(PreambleCallbacks ) : Callbacks(Callbacks) {}
+
+ void MacroDefined(const Token ,
+const MacroDirective *MD) override {
+Callbacks.HandleMacroDefined(MacroNameTok, MD);
+ }
+
+private:
+ PreambleCallbacks
+};
+
+class PrecompilePreambleAction : public ASTFrontendAction {
+public:
+ PrecompilePreambleAction(PreambleCallbacks )
+ : Callbacks(Callbacks) {}
+
+ std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef InFile) override;
+
+ bool hasEmittedPreamblePCH() const { return HasEmittedPreamblePCH; }
+
+ void setEmittedPreamblePCH(ASTWriter ) {
+this->HasEmittedPreamblePCH = true;
+Callbacks.AfterPCHEmitted(Writer);
+ }
+
+ bool shouldEraseOutputFiles() override { return !hasEmittedPreamblePCH(); }
+ bool hasCodeCompletionSupport() const override { return false; }
+ bool hasASTFileSupport() const override { return false; }
+ TranslationUnitKind getTranslationUnitKind() override { return TU_Prefix; }
+
+private:
+ friend class PrecompilePreambleConsumer;
+
+ bool HasEmittedPreamblePCH = false;
+ PreambleCallbacks
+};
+
+class PrecompilePreambleConsumer : public PCHGenerator {
+public:
+ PrecompilePreambleConsumer(PrecompilePreambleAction ,
+ const Preprocessor , StringRef isysroot,
+ std::unique_ptr Out)
+ : PCHGenerator(PP, "", isysroot, std::make_shared(),
+ ArrayRef(),
+ /*AllowASTWithErrors=*/true),
+Action(Action), Out(std::move(Out)) {}
+
+ bool HandleTopLevelDecl(DeclGroupRef DG) override {
+Action.Callbacks.HandleTopLevelDecl(DG);
+
[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.
klimek added inline comments.
Comment at: include/clang/Frontend/PrecompiledPreamble.h:124
+/// CanReusePreamble + AddImplicitPreamble to make use of it.
+class PrecompiledPreamble {
+public:
ilya-biryukov wrote:
> 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).
TempPCHFile looks like something we just want to put into the .cc file and
store as a unique_ptr.
PreambleFileHash seems fine as an extra header.
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 ,
ilya-biryukov wrote:
> 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?
Generally, if these are closely coupled to implementation details of
PrecompiledPreample, I think that coupling is strong enough to warrant making
them members.
On the other hand, making some functions members, and others non-members, and
putting them next to each other in the .cc file also would work.
https://reviews.llvm.org/D34287
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.
ilya-biryukov updated this revision to Diff 103203.
ilya-biryukov added a comment.
Removed PossiblyOwnedBuffer, added an extra copy instead.
This makes the code much simpler.
https://reviews.llvm.org/D34287
Files:
include/clang/Frontend/ASTUnit.h
include/clang/Frontend/PrecompiledPreamble.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/CMakeLists.txt
lib/Frontend/PrecompiledPreamble.cpp
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- /dev/null
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -0,0 +1,573 @@
+//===--- PrecompiledPreamble.cpp - Build precompiled preambles --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Helper class to build precompiled preamble.
+//
+//===--===//
+
+#include "clang/Frontend/PrecompiledPreamble.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Serialization/ASTWriter.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/Support/CrashRecoveryContext.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/MutexGuard.h"
+
+using namespace clang;
+
+namespace {
+
+/// Keeps a track of files to be deleted in destructor.
+class TemporaryFiles {
+public:
+ // A static instance to be used by
+ static TemporaryFiles ();
+
+private:
+ // Disallow constructing the class directly.
+ TemporaryFiles() = default;
+ // Disallow copy.
+ TemporaryFiles(const TemporaryFiles &) = delete;
+
+public:
+ ~TemporaryFiles();
+
+ /// Adds \p File to a set of tracked files.
+ void addFile(StringRef File);
+
+ /// Remove \p File from disk and from the set of tracked files.
+ void removeFile(StringRef File);
+
+private:
+ llvm::sys::SmartMutex Mutex;
+ llvm::StringSet<> Files;
+};
+
+TemporaryFiles ::getInstance() {
+ static TemporaryFiles Instance;
+ return Instance;
+}
+
+TemporaryFiles::~TemporaryFiles() {
+ llvm::MutexGuard Guard(Mutex);
+ for (const auto : Files)
+llvm::sys::fs::remove(File.getKey());
+}
+
+void TemporaryFiles::addFile(StringRef File) {
+ llvm::MutexGuard Guard(Mutex);
+ auto IsInserted = Files.insert(File).second;
+ assert(IsInserted && "File has already been added");
+}
+
+void TemporaryFiles::removeFile(StringRef File) {
+ llvm::MutexGuard Guard(Mutex);
+ auto WasPresent = Files.erase(File);
+ assert(WasPresent && "File was not tracked");
+ llvm::sys::fs::remove(File);
+}
+
+class PreambleMacroCallbacks : public PPCallbacks {
+public:
+ PreambleMacroCallbacks(PreambleCallbacks ) : Callbacks(Callbacks) {}
+
+ void MacroDefined(const Token ,
+const MacroDirective *MD) override {
+Callbacks.HandleMacroDefined(MacroNameTok, MD);
+ }
+
+private:
+ PreambleCallbacks
+};
+
+class PrecompilePreambleAction : public ASTFrontendAction {
+public:
+ PrecompilePreambleAction(PreambleCallbacks )
+ : Callbacks(Callbacks) {}
+
+ std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef InFile) override;
+
+ bool hasEmittedPreamblePCH() const { return HasEmittedPreamblePCH; }
+
+ void setEmittedPreamblePCH(ASTWriter ) {
+this->HasEmittedPreamblePCH = true;
+Callbacks.AfterPCHEmitted(Writer);
+ }
+
+ bool shouldEraseOutputFiles() override { return !hasEmittedPreamblePCH(); }
+ bool hasCodeCompletionSupport() const override { return false; }
+ bool hasASTFileSupport() const override { return false; }
+ TranslationUnitKind getTranslationUnitKind() override { return TU_Prefix; }
+
+private:
+ friend class PrecompilePreambleConsumer;
+
+ bool HasEmittedPreamblePCH = false;
+ PreambleCallbacks
+};
+
+class PrecompilePreambleConsumer : public PCHGenerator {
+public:
+ PrecompilePreambleConsumer(PrecompilePreambleAction ,
+ const Preprocessor , StringRef isysroot,
+ std::unique_ptr Out)
+ : PCHGenerator(PP, "", isysroot, std::make_shared(),
+ ArrayRef(),
+ /*AllowASTWithErrors=*/true),
+Action(Action), Out(std::move(Out)) {}
+
+ bool HandleTopLevelDecl(DeclGroupRef DG) override {
+Action.Callbacks.HandleTopLevelDecl(DG);
+return true;
+ }
+
+ void HandleTranslationUnit(ASTContext ) override {
+PCHGenerator::HandleTranslationUnit(Ctx);
+if
[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.
klimek added inline comments.
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 ,
+ vfs::FileSystem *VFS,
+ StringRef FilePath) {
ilya-biryukov wrote:
> 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.
After some in person discussion, the idea now is to just always copy and return
a unique_ptr
https://reviews.llvm.org/D34287
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.
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 ,
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 () {
- static llvm::sys::SmartMutex 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` or accept an out parameter `unique_ptr `
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 ,
+ 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
[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.
ilya-biryukov updated this revision to Diff 103053.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.
- Added default initializers to PreambleFileHash.
- Update file comments to address klimek's questions.
https://reviews.llvm.org/D34287
Files:
include/clang/Frontend/ASTUnit.h
include/clang/Frontend/PrecompiledPreamble.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/CMakeLists.txt
lib/Frontend/PrecompiledPreamble.cpp
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- /dev/null
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -0,0 +1,573 @@
+//===--- PrecompiledPreamble.cpp - Build precompiled preambles --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Helper class to build precompiled preamble.
+//
+//===--===//
+
+#include "clang/Frontend/PrecompiledPreamble.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Serialization/ASTWriter.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/Support/CrashRecoveryContext.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/MutexGuard.h"
+
+using namespace clang;
+
+namespace {
+
+/// Keeps a track of files to be deleted in destructor.
+class TemporaryFiles {
+public:
+ // A static instance to be used by
+ static TemporaryFiles ();
+
+private:
+ // Disallow constructing the class directly.
+ TemporaryFiles() = default;
+ // Disallow copy.
+ TemporaryFiles(const TemporaryFiles &) = delete;
+
+public:
+ ~TemporaryFiles();
+
+ /// Adds \p File to a set of tracked files.
+ void addFile(StringRef File);
+
+ /// Remove \p File from disk and from the set of tracked files.
+ void removeFile(StringRef File);
+
+private:
+ llvm::sys::SmartMutex Mutex;
+ llvm::StringSet<> Files;
+};
+
+TemporaryFiles ::getInstance() {
+ static TemporaryFiles Instance;
+ return Instance;
+}
+
+TemporaryFiles::~TemporaryFiles() {
+ llvm::MutexGuard Guard(Mutex);
+ for (const auto : Files)
+llvm::sys::fs::remove(File.getKey());
+}
+
+void TemporaryFiles::addFile(StringRef File) {
+ llvm::MutexGuard Guard(Mutex);
+ auto IsInserted = Files.insert(File).second;
+ assert(IsInserted && "File has already been added");
+}
+
+void TemporaryFiles::removeFile(StringRef File) {
+ llvm::MutexGuard Guard(Mutex);
+ auto WasPresent = Files.erase(File);
+ assert(WasPresent && "File was not tracked");
+ llvm::sys::fs::remove(File);
+}
+
+class PreambleMacroCallbacks : public PPCallbacks {
+public:
+ PreambleMacroCallbacks(PreambleCallbacks ) : Callbacks(Callbacks) {}
+
+ void MacroDefined(const Token ,
+const MacroDirective *MD) override {
+Callbacks.HandleMacroDefined(MacroNameTok, MD);
+ }
+
+private:
+ PreambleCallbacks
+};
+
+class PrecompilePreambleAction : public ASTFrontendAction {
+public:
+ PrecompilePreambleAction(PreambleCallbacks )
+ : Callbacks(Callbacks) {}
+
+ std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef InFile) override;
+
+ bool hasEmittedPreamblePCH() const { return HasEmittedPreamblePCH; }
+
+ void setEmittedPreamblePCH(ASTWriter ) {
+this->HasEmittedPreamblePCH = true;
+Callbacks.AfterPCHEmitted(Writer);
+ }
+
+ bool shouldEraseOutputFiles() override { return !hasEmittedPreamblePCH(); }
+ bool hasCodeCompletionSupport() const override { return false; }
+ bool hasASTFileSupport() const override { return false; }
+ TranslationUnitKind getTranslationUnitKind() override { return TU_Prefix; }
+
+private:
+ friend class PrecompilePreambleConsumer;
+
+ bool HasEmittedPreamblePCH = false;
+ PreambleCallbacks
+};
+
+class PrecompilePreambleConsumer : public PCHGenerator {
+public:
+ PrecompilePreambleConsumer(PrecompilePreambleAction ,
+ const Preprocessor , StringRef isysroot,
+ std::unique_ptr Out)
+ : PCHGenerator(PP, "", isysroot, std::make_shared(),
+ ArrayRef(),
+ /*AllowASTWithErrors=*/true),
+Action(Action), Out(std::move(Out)) {}
+
+ bool HandleTopLevelDecl(DeclGroupRef DG) override {
+Action.Callbacks.HandleTopLevelDecl(DG);
+return true;
+ }
+
+ void HandleTranslationUnit(ASTContext ) override
[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.
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 ,
Why not make it a member instead?
Comment at: lib/Frontend/ASTUnit.cpp:98
-static llvm::sys::SmartMutex () {
- static llvm::sys::SmartMutex 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 ,
+ 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
[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.
ilya-biryukov added inline comments. Comment at: include/clang/Frontend/PrecompiledPreamble.h:248 +/// doesn't restore the state \p CI had before calling AddImplicitPreamble, only +/// clears relevant settings, so that preamble is disabled in \p CI. +} // namespace clang arphaman wrote: > Is there a missing declaration here or is this a comment for another > declaration? Indeed, I forgot to remove a comment after I removed a declaration. Thanks for spotting that. https://reviews.llvm.org/D34287 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.
ilya-biryukov updated this revision to Diff 102993.
ilya-biryukov added a comment.
Removed a stray comment (of a previously removed declaration).
https://reviews.llvm.org/D34287
Files:
include/clang/Frontend/ASTUnit.h
include/clang/Frontend/PrecompiledPreamble.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/CMakeLists.txt
lib/Frontend/PrecompiledPreamble.cpp
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- /dev/null
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -0,0 +1,573 @@
+//===--- PrecompiledPreamble.cpp - Build precompiled preambles --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Helper class to build precompiled preamble.
+//
+//===--===//
+
+#include "clang/Frontend/PrecompiledPreamble.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Serialization/ASTWriter.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/Support/CrashRecoveryContext.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/MutexGuard.h"
+
+using namespace clang;
+
+namespace {
+
+/// Keeps a track of files to be deleted in destructor.
+class TemporaryFiles {
+public:
+ // A static instance to be used by
+ static TemporaryFiles ();
+
+private:
+ // Disallow constructing the class directly.
+ TemporaryFiles() = default;
+ // Disallow copy.
+ TemporaryFiles(const TemporaryFiles &) = delete;
+
+public:
+ ~TemporaryFiles();
+
+ /// Adds \p File to a set of tracked files.
+ void addFile(StringRef File);
+
+ /// Remove \p File from disk and from the set of tracked files.
+ void removeFile(StringRef File);
+
+private:
+ llvm::sys::SmartMutex Mutex;
+ llvm::StringSet<> Files;
+};
+
+TemporaryFiles ::getInstance() {
+ static TemporaryFiles Instance;
+ return Instance;
+}
+
+TemporaryFiles::~TemporaryFiles() {
+ llvm::MutexGuard Guard(Mutex);
+ for (const auto : Files)
+llvm::sys::fs::remove(File.getKey());
+}
+
+void TemporaryFiles::addFile(StringRef File) {
+ llvm::MutexGuard Guard(Mutex);
+ auto IsInserted = Files.insert(File).second;
+ assert(IsInserted && "File has already been added");
+}
+
+void TemporaryFiles::removeFile(StringRef File) {
+ llvm::MutexGuard Guard(Mutex);
+ auto WasPresent = Files.erase(File);
+ assert(WasPresent && "File was not tracked");
+ llvm::sys::fs::remove(File);
+}
+
+class PreambleMacroCallbacks : public PPCallbacks {
+public:
+ PreambleMacroCallbacks(PreambleCallbacks ) : Callbacks(Callbacks) {}
+
+ void MacroDefined(const Token ,
+const MacroDirective *MD) override {
+Callbacks.HandleMacroDefined(MacroNameTok, MD);
+ }
+
+private:
+ PreambleCallbacks
+};
+
+class PrecompilePreambleAction : public ASTFrontendAction {
+public:
+ PrecompilePreambleAction(PreambleCallbacks )
+ : Callbacks(Callbacks) {}
+
+ std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef InFile) override;
+
+ bool hasEmittedPreamblePCH() const { return HasEmittedPreamblePCH; }
+
+ void setEmittedPreamblePCH(ASTWriter ) {
+this->HasEmittedPreamblePCH = true;
+Callbacks.AfterPCHEmitted(Writer);
+ }
+
+ bool shouldEraseOutputFiles() override { return !hasEmittedPreamblePCH(); }
+ bool hasCodeCompletionSupport() const override { return false; }
+ bool hasASTFileSupport() const override { return false; }
+ TranslationUnitKind getTranslationUnitKind() override { return TU_Prefix; }
+
+private:
+ friend class PrecompilePreambleConsumer;
+
+ bool HasEmittedPreamblePCH = false;
+ PreambleCallbacks
+};
+
+class PrecompilePreambleConsumer : public PCHGenerator {
+public:
+ PrecompilePreambleConsumer(PrecompilePreambleAction ,
+ const Preprocessor , StringRef isysroot,
+ std::unique_ptr Out)
+ : PCHGenerator(PP, "", isysroot, std::make_shared(),
+ ArrayRef(),
+ /*AllowASTWithErrors=*/true),
+Action(Action), Out(std::move(Out)) {}
+
+ bool HandleTopLevelDecl(DeclGroupRef DG) override {
+Action.Callbacks.HandleTopLevelDecl(DG);
+return true;
+ }
+
+ void HandleTranslationUnit(ASTContext ) override {
+PCHGenerator::HandleTranslationUnit(Ctx);
+if (!hasEmittedPCH())
+
[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.
arphaman added inline comments. Comment at: include/clang/Frontend/PrecompiledPreamble.h:248 +/// doesn't restore the state \p CI had before calling AddImplicitPreamble, only +/// clears relevant settings, so that preamble is disabled in \p CI. +} // namespace clang Is there a missing declaration here or is this a comment for another declaration? https://reviews.llvm.org/D34287 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.
ilya-biryukov created this revision.
Herald added a subscriber: mgorny.
https://reviews.llvm.org/D34287
Files:
include/clang/Frontend/ASTUnit.h
include/clang/Frontend/PrecompiledPreamble.h
lib/Frontend/ASTUnit.cpp
lib/Frontend/CMakeLists.txt
lib/Frontend/PrecompiledPreamble.cpp
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- /dev/null
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -0,0 +1,573 @@
+//===--- PrecompiledPreamble.cpp - Build precompiled preambles --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Helper class to build precompiled preamble.
+//
+//===--===//
+
+#include "clang/Frontend/PrecompiledPreamble.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Serialization/ASTWriter.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/Support/CrashRecoveryContext.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/MutexGuard.h"
+
+using namespace clang;
+
+namespace {
+
+/// Keeps a track of files to be deleted in destructor.
+class TemporaryFiles {
+public:
+ // A static instance to be used by
+ static TemporaryFiles ();
+
+private:
+ // Disallow constructing the class directly.
+ TemporaryFiles() = default;
+ // Disallow copy.
+ TemporaryFiles(const TemporaryFiles &) = delete;
+
+public:
+ ~TemporaryFiles();
+
+ /// Adds \p File to a set of tracked files.
+ void addFile(StringRef File);
+
+ /// Remove \p File from disk and from the set of tracked files.
+ void removeFile(StringRef File);
+
+private:
+ llvm::sys::SmartMutex Mutex;
+ llvm::StringSet<> Files;
+};
+
+TemporaryFiles ::getInstance() {
+ static TemporaryFiles Instance;
+ return Instance;
+}
+
+TemporaryFiles::~TemporaryFiles() {
+ llvm::MutexGuard Guard(Mutex);
+ for (const auto : Files)
+llvm::sys::fs::remove(File.getKey());
+}
+
+void TemporaryFiles::addFile(StringRef File) {
+ llvm::MutexGuard Guard(Mutex);
+ auto IsInserted = Files.insert(File).second;
+ assert(IsInserted && "File has already been added");
+}
+
+void TemporaryFiles::removeFile(StringRef File) {
+ llvm::MutexGuard Guard(Mutex);
+ auto WasPresent = Files.erase(File);
+ assert(WasPresent && "File was not tracked");
+ llvm::sys::fs::remove(File);
+}
+
+class PreambleMacroCallbacks : public PPCallbacks {
+public:
+ PreambleMacroCallbacks(PreambleCallbacks ) : Callbacks(Callbacks) {}
+
+ void MacroDefined(const Token ,
+const MacroDirective *MD) override {
+Callbacks.HandleMacroDefined(MacroNameTok, MD);
+ }
+
+private:
+ PreambleCallbacks
+};
+
+class PrecompilePreambleAction : public ASTFrontendAction {
+public:
+ PrecompilePreambleAction(PreambleCallbacks )
+ : Callbacks(Callbacks) {}
+
+ std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef InFile) override;
+
+ bool hasEmittedPreamblePCH() const { return HasEmittedPreamblePCH; }
+
+ void setEmittedPreamblePCH(ASTWriter ) {
+this->HasEmittedPreamblePCH = true;
+Callbacks.AfterPCHEmitted(Writer);
+ }
+
+ bool shouldEraseOutputFiles() override { return !hasEmittedPreamblePCH(); }
+ bool hasCodeCompletionSupport() const override { return false; }
+ bool hasASTFileSupport() const override { return false; }
+ TranslationUnitKind getTranslationUnitKind() override { return TU_Prefix; }
+
+private:
+ friend class PrecompilePreambleConsumer;
+
+ bool HasEmittedPreamblePCH = false;
+ PreambleCallbacks
+};
+
+class PrecompilePreambleConsumer : public PCHGenerator {
+public:
+ PrecompilePreambleConsumer(PrecompilePreambleAction ,
+ const Preprocessor , StringRef isysroot,
+ std::unique_ptr Out)
+ : PCHGenerator(PP, "", isysroot, std::make_shared(),
+ ArrayRef(),
+ /*AllowASTWithErrors=*/true),
+Action(Action), Out(std::move(Out)) {}
+
+ bool HandleTopLevelDecl(DeclGroupRef DG) override {
+Action.Callbacks.HandleTopLevelDecl(DG);
+return true;
+ }
+
+ void HandleTranslationUnit(ASTContext ) override {
+PCHGenerator::HandleTranslationUnit(Ctx);
+if (!hasEmittedPCH())
+ return;
+
+// Write the generated bitstream to "Out".
+*Out << getPCH();
