Make DiagnosticsEngine::takeClient return std::unique_ptr<>

Summary:
Make DiagnosticsEngine::takeClient return std::unique_ptr<>. Updated
callers to store conditional ownership using a pair of pointer and unique_ptr
instead of a pointer + bool. Updated code that temporarily registers clients to
use the non-owning registration (+ removed extra calls to takeClient).

Reviewers: dblaikie

Reviewed By: dblaikie

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D6294

llvm-svn: 222193

clang/include/clang/Basic/Diagnostic.h

@@ -187,7 +187,7 @@ private:
   IntrusiveRefCntPtr<DiagnosticIDs> Diags;
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts;
   DiagnosticConsumer *Client;
-  bool OwnsDiagClient;
+  std::unique_ptr<DiagnosticConsumer> Owner;
   SourceManager *SourceMgr;
 
   /// \brief Mapping information for diagnostics.
@@ -347,7 +347,6 @@ public:
                       DiagnosticOptions *DiagOpts,
                       DiagnosticConsumer *client = nullptr,
                       bool ShouldOwnClient = true);
-  ~DiagnosticsEngine();
 
   const IntrusiveRefCntPtr<DiagnosticIDs> &getDiagnosticIDs() const {
     return Diags;
@@ -368,14 +367,11 @@ public:
   const DiagnosticConsumer *getClient() const { return Client; }
 
   /// \brief Determine whether this \c DiagnosticsEngine object own its client.
-  bool ownsClient() const { return OwnsDiagClient; }
-  
+  bool ownsClient() const { return Owner != nullptr; }
+
   /// \brief Return the current diagnostic client along with ownership of that
   /// client.
-  DiagnosticConsumer *takeClient() {
-    OwnsDiagClient = false;
-    return Client;
-  }
+  std::unique_ptr<DiagnosticConsumer> takeClient() { return std::move(Owner); }
 
   bool hasSourceManager() const { return SourceMgr != nullptr; }
   SourceManager &getSourceManager() const {

clang/include/clang/Frontend/VerifyDiagnosticConsumer.h

@@ -212,7 +212,7 @@ public:
 private:
   DiagnosticsEngine &Diags;
   DiagnosticConsumer *PrimaryClient;
-  bool OwnsPrimaryClient;
+  std::unique_ptr<DiagnosticConsumer> PrimaryClientOwner;
   std::unique_ptr<TextDiagnosticBuffer> Buffer;
   const Preprocessor *CurrentPreprocessor;
   const LangOptions *LangOpts;

clang/include/clang/Rewrite/Frontend/FixItRewriter.h

@@ -66,7 +66,7 @@ class FixItRewriter : public DiagnosticConsumer {
   /// \brief The diagnostic client that performs the actual formatting
   /// of error messages.
   DiagnosticConsumer *Client;
-  bool OwnsClient;
+  std::unique_ptr<DiagnosticConsumer> Owner;
 
   /// \brief Turn an input path into an output path. NULL implies overwriting
   /// the original.

clang/lib/Basic/Diagnostic.cpp

@@ -33,13 +33,11 @@ static void DummyArgToStringFn(DiagnosticsEngine::ArgumentKind AK, intptr_t QT,
   Output.append(Str.begin(), Str.end());
 }
 
-
 DiagnosticsEngine::DiagnosticsEngine(
-                       const IntrusiveRefCntPtr<DiagnosticIDs> &diags,
-                       DiagnosticOptions *DiagOpts,       
-                       DiagnosticConsumer *client, bool ShouldOwnClient)
-  : Diags(diags), DiagOpts(DiagOpts), Client(client),
-    OwnsDiagClient(ShouldOwnClient), SourceMgr(nullptr) {
+    const IntrusiveRefCntPtr<DiagnosticIDs> &diags, DiagnosticOptions *DiagOpts,
+    DiagnosticConsumer *client, bool ShouldOwnClient)
+    : Diags(diags), DiagOpts(DiagOpts), Client(nullptr), SourceMgr(nullptr) {
+  setClient(client, ShouldOwnClient);
   ArgToStringFn = DummyArgToStringFn;
   ArgToStringCookie = nullptr;
 
@@ -63,18 +61,10 @@ DiagnosticsEngine::DiagnosticsEngine(
   Reset();
 }
 
-DiagnosticsEngine::~DiagnosticsEngine() {
-  if (OwnsDiagClient)
-    delete Client;
-}
-
 void DiagnosticsEngine::setClient(DiagnosticConsumer *client,
                                   bool ShouldOwnClient) {
-  if (OwnsDiagClient && Client)
-    delete Client;
-  
+  Owner.reset(ShouldOwnClient ? client : nullptr);
   Client = client;
-  OwnsDiagClient = ShouldOwnClient;
 }
 
 void DiagnosticsEngine::pushMappings(SourceLocation Loc) {

clang/lib/Frontend/ASTUnit.cpp

@@ -589,6 +589,7 @@ class CaptureDroppedDiagnostics {
   DiagnosticsEngine &Diags;
   StoredDiagnosticConsumer Client;
   DiagnosticConsumer *PreviousClient;
+  std::unique_ptr<DiagnosticConsumer> OwningPreviousClient;
 
 public:
   CaptureDroppedDiagnostics(bool RequestCapture, DiagnosticsEngine &Diags,
@@ -596,16 +597,15 @@ public:
     : Diags(Diags), Client(StoredDiags), PreviousClient(nullptr)
   {
     if (RequestCapture || Diags.getClient() == nullptr) {
-      PreviousClient = Diags.takeClient();
-      Diags.setClient(&Client);
+      OwningPreviousClient = Diags.takeClient();
+      PreviousClient = Diags.getClient();
+      Diags.setClient(&Client, false);
     }
   }
 
   ~CaptureDroppedDiagnostics() {
-    if (Diags.getClient() == &Client) {
-      Diags.takeClient();
-      Diags.setClient(PreviousClient);
-    }
+    if (Diags.getClient() == &Client)
+      Diags.setClient(PreviousClient, !!OwningPreviousClient.release());
   }
 };
 

clang/lib/Frontend/CompilerInstance.cpp

@@ -159,9 +159,8 @@ static void SetUpDiagnosticLog(DiagnosticOptions *DiagOpts,
   if (CodeGenOpts)
     Logger->setDwarfDebugFlags(CodeGenOpts->DwarfDebugFlags);
   assert(Diags.ownsClient());
-  Diags.setClient(new ChainedDiagnosticConsumer(
-      std::unique_ptr<DiagnosticConsumer>(Diags.takeClient()),
-      std::move(Logger)));
+  Diags.setClient(
+      new ChainedDiagnosticConsumer(Diags.takeClient(), std::move(Logger)));
 }
 
 static void SetupSerializedDiagnostics(DiagnosticOptions *DiagOpts,
@@ -172,8 +171,7 @@ static void SetupSerializedDiagnostics(DiagnosticOptions *DiagOpts,
 
   if (Diags.ownsClient()) {
     Diags.setClient(new ChainedDiagnosticConsumer(
-        std::unique_ptr<DiagnosticConsumer>(Diags.takeClient()),
-        std::move(SerializedConsumer)));
+        Diags.takeClient(), std::move(SerializedConsumer)));
   } else {
     Diags.setClient(new ChainedDiagnosticConsumer(
         Diags.getClient(), std::move(SerializedConsumer)));

clang/lib/Frontend/Rewrite/FixItRewriter.cpp

@@ -36,14 +36,13 @@ FixItRewriter::FixItRewriter(DiagnosticsEngine &Diags, SourceManager &SourceMgr,
     FixItOpts(FixItOpts),
     NumFailures(0),
     PrevDiagSilenced(false) {
-  OwnsClient = Diags.ownsClient();
-  Client = Diags.takeClient();
-  Diags.setClient(this);
+  Owner = Diags.takeClient();
+  Client = Diags.getClient();
+  Diags.setClient(this, false);
 }
 
 FixItRewriter::~FixItRewriter() {
-  Diags.takeClient();
-  Diags.setClient(Client, OwnsClient);
+  Diags.setClient(Client, Owner.release() != nullptr);
 }
 
 bool FixItRewriter::WriteFixedFile(FileID ID, raw_ostream &OS) {
@@ -188,12 +187,10 @@ void FixItRewriter::Diag(SourceLocation Loc, unsigned DiagID) {
   // When producing this diagnostic, we temporarily bypass ourselves,
   // clear out any current diagnostic, and let the downstream client
   // format the diagnostic.
-  Diags.takeClient();
-  Diags.setClient(Client);
+  Diags.setClient(Client, false);
   Diags.Clear();
   Diags.Report(Loc, DiagID);
-  Diags.takeClient();
-  Diags.setClient(this);
+  Diags.setClient(this, false);
 }
 
 FixItOptions::~FixItOptions() {}

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

@@ -29,12 +29,11 @@ typedef VerifyDiagnosticConsumer::ExpectedData ExpectedData;
 
 VerifyDiagnosticConsumer::VerifyDiagnosticConsumer(DiagnosticsEngine &Diags_)
   : Diags(Diags_),
-    PrimaryClient(Diags.getClient()), OwnsPrimaryClient(Diags.ownsClient()),
+    PrimaryClient(Diags.getClient()), PrimaryClientOwner(Diags.takeClient()),
     Buffer(new TextDiagnosticBuffer()), CurrentPreprocessor(nullptr),
     LangOpts(nullptr), SrcManager(nullptr), ActiveSourceFiles(0),
     Status(HasNoDirectives)
 {
-  Diags.takeClient();
   if (Diags.hasSourceManager())
     setSourceManager(Diags.getSourceManager());
 }
@@ -43,10 +42,8 @@ VerifyDiagnosticConsumer::~VerifyDiagnosticConsumer() {
   assert(!ActiveSourceFiles && "Incomplete parsing of source files!");
   assert(!CurrentPreprocessor && "CurrentPreprocessor should be invalid!");
   SrcManager = nullptr;
-  CheckDiagnostics();  
-  Diags.takeClient();
-  if (OwnsPrimaryClient)
-    delete PrimaryClient;
+  CheckDiagnostics();
+  Diags.takeClient().release();
 }
 
 #ifndef NDEBUG
@@ -802,8 +799,8 @@ void VerifyDiagnosticConsumer::UpdateParsedFileStatus(SourceManager &SM,
 
 void VerifyDiagnosticConsumer::CheckDiagnostics() {
   // Ensure any diagnostics go to the primary client.
-  bool OwnsCurClient = Diags.ownsClient();
-  DiagnosticConsumer *CurClient = Diags.takeClient();
+  DiagnosticConsumer *CurClient = Diags.getClient();
+  std::unique_ptr<DiagnosticConsumer> Owner = Diags.takeClient();
   Diags.setClient(PrimaryClient, false);
 
 #ifndef NDEBUG
@@ -865,8 +862,7 @@ void VerifyDiagnosticConsumer::CheckDiagnostics() {
                                   Buffer->note_end(), "note"));
   }
 
-  Diags.takeClient();
-  Diags.setClient(CurClient, OwnsCurClient);
+  Diags.setClient(CurClient, Owner.release() != nullptr);
 
   // Reset the buffer, we have processed all the diagnostics in it.
   Buffer.reset(new TextDiagnosticBuffer());

clang/tools/driver/driver.cpp

@@ -451,8 +451,7 @@ int main(int argc_, const char **argv_) {
         clang::serialized_diags::create(DiagOpts->DiagnosticSerializationFile,
                                         &*DiagOpts, /*MergeChildRecords=*/true);
     Diags.setClient(new ChainedDiagnosticConsumer(
-        std::unique_ptr<DiagnosticConsumer>(Diags.takeClient()),
-        std::move(SerializedConsumer)));
+        Diags.takeClient(), std::move(SerializedConsumer)));
   }
 
   ProcessWarningOptions(Diags, *DiagOpts, /*ReportDiags=*/false);

clang/unittests/Sema/ExternalSemaSourceTest.cpp

@@ -154,7 +154,7 @@ protected:
     DiagnosticsEngine &Diagnostics = CI.getDiagnostics();
     DiagnosticConsumer *Client = Diagnostics.getClient();
     if (Diagnostics.ownsClient())
-      OwnedClient.reset(Diagnostics.takeClient());
+      OwnedClient = Diagnostics.takeClient();
     for (size_t I = 0, E = Watchers.size(); I < E; ++I)
       Client = Watchers[I]->Chain(Client);
     Diagnostics.setClient(Client, false);