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

[alexfh]

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).

[dblaikie]

Looks mostly like a sort-of improvement, with a bit more tidying up that could be done in follow-up patches (though I'd slightly prefer to drop the !! before committing this at least, and double-check the owner naming convention in existing cases to be consistent)

include/clang/Basic/Diagnostic.h

370

I don't think !! is idiomatic in the LLVM codebase (though I may be incorrect) & probably just (bool) or static_cast<bool> would be more consistent.

include/clang/Rewrite/Frontend/FixItRewriter.h

69

What's the naming convention for other T*+unique_ptr<T> combos like this? I think I introduced the first few of these (see the email thread on the "maybe owning" pointer semantics in llvm-dev/cfe-dev for a list of other cases) and I might've used "*Owner" rather than "Owning*" - it'd be nice to be consistent (so they're easy to find and cleanup later).

lib/Frontend/Rewrite/FixItRewriter.cpp

45

As mentioned above (hmm, should this class use the CaptureDroppedDiagnostics? I guess not (it probably does other things) - but maybe we should have a common scoped "replace the DiagnosticConsumer" device that could be used in both places? - though that's probably overkill once we fix the ownership one way or another)

lib/Frontend/VerifyDiagnosticConsumer.cpp

46

While it didn't have it before - this might deserve a comment.

[alexfh]

include/clang/Basic/Diagnostic.h

370

Changed "!!" to "!= nullptr".

include/clang/Rewrite/Frontend/FixItRewriter.h

69

Renamed Owning* to *Owner or just Owner depending on wether the latter is ambiguous in the context or not.

lib/Frontend/Rewrite/FixItRewriter.cpp

45

A separate scoped diagnostic client replacer would be nice, but it's slightly out of scope for this patch.

lib/Frontend/VerifyDiagnosticConsumer.cpp

46

I'm not sure I completely understand what's going on here. I'm actually not sure if this code is even correct. I just attempted to leave the existing behavior here. So I'd refrain from writing a misleading comment until I figure this out.

include/clang/Basic/Diagnostic.h

private:
   unsigned TemplateBacktraceLimit; // Cap on depth of template backtrace stack,
                                    // 0 -> no limit.
   unsigned ConstexprBacktraceLimit; // Cap on depth of constexpr evaluation
                                     // backtrace stack, 0 -> no limit.
   diag::Severity ExtBehavior;       // Map extensions to warnings or errors?
   IntrusiveRefCntPtr<DiagnosticIDs> Diags;
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts;
   DiagnosticConsumer *Client;
-  bool OwnsDiagClient;
+  std::unique_ptr<DiagnosticConsumer> OwningClient;
   SourceManager *SourceMgr;
 
   /// \brief Mapping information for diagnostics.
   ///
   /// Mapping info is packed into four bits per diagnostic.  The low three
   /// bits are the mapping (an instance of diag::Severity), or zero if unset.
   /// The high bit is set when the mapping was established as a user mapping.
   /// If the high bit is clear, then the low bits are set to the default
private:
   std::string FlagValue;
 
 public:
   explicit DiagnosticsEngine(
                       const IntrusiveRefCntPtr<DiagnosticIDs> &Diags,
                       DiagnosticOptions *DiagOpts,
                       DiagnosticConsumer *client = nullptr,
                       bool ShouldOwnClient = true);
-  ~DiagnosticsEngine();
 
   const IntrusiveRefCntPtr<DiagnosticIDs> &getDiagnosticIDs() const {
     return Diags;
   }
 
   /// \brief Retrieve the diagnostic options.
   DiagnosticOptions &getDiagnosticOptions() const { return *DiagOpts; }
 
   typedef llvm::iterator_range<DiagState::const_iterator> diag_mapping_range;
 
   /// \brief Get the current set of diagnostic mappings.
   diag_mapping_range getDiagnosticMappings() const {
     const DiagState &DS = *GetCurDiagState();
     return diag_mapping_range(DS.begin(), DS.end());
   }
 
   DiagnosticConsumer *getClient() { return Client; }
   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 !!OwningClient; }
   /// \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(OwningClient);
   }
 
   bool hasSourceManager() const { return SourceMgr != nullptr; }
   SourceManager &getSourceManager() const {
     assert(SourceMgr && "SourceManager not set!");
     return *SourceMgr;
   }
   void setSourceManager(SourceManager *SrcMgr) { SourceMgr = SrcMgr; }

include/clang/Frontend/VerifyDiagnosticConsumer.h

enum DirectiveStatus {
     HasNoDirectivesReported,
     HasExpectedNoDiagnostics,
     HasOtherExpectedDirectives
   };
 
 private:
   DiagnosticsEngine &Diags;
   DiagnosticConsumer *PrimaryClient;
-  bool OwnsPrimaryClient;
+  std::unique_ptr<DiagnosticConsumer> OwningPrimaryClient;
   std::unique_ptr<TextDiagnosticBuffer> Buffer;
   const Preprocessor *CurrentPreprocessor;
   const LangOptions *LangOpts;
   SourceManager *SrcManager;
   unsigned ActiveSourceFiles;
   DirectiveStatus Status;
   ExpectedData ED;
 

include/clang/Rewrite/Frontend/FixItRewriter.h

class FixItRewriter : public DiagnosticConsumer {
 
   /// \brief The rewriter used to perform the various code
   /// modifications.
   Rewriter Rewrite;
 
   /// \brief The diagnostic client that performs the actual formatting
   /// of error messages.
   DiagnosticConsumer *Client;
-  bool OwnsClient;
+  std::unique_ptr<DiagnosticConsumer> OwningClient;
 
   /// \brief Turn an input path into an output path. NULL implies overwriting
   /// the original.
   FixItOptions *FixItOpts;
 
   /// \brief The number of rewriter failures.
   unsigned NumFailures;
 

lib/Basic/Diagnostic.cpp

static void DummyArgToStringFn(DiagnosticsEngine::ArgumentKind AK, intptr_t QT,
                             ArrayRef<DiagnosticsEngine::ArgumentValue> PrevArgs,
                             SmallVectorImpl<char> &Output,
                             void *Cookie,
                             ArrayRef<intptr_t> QualTypeVals) {
   StringRef Str = "<can't format argument>";
   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,
+    : Diags(diags), DiagOpts(DiagOpts), Client(nullptr), SourceMgr(nullptr) {
+  setClient(client, ShouldOwnClient);
   ArgToStringFn = DummyArgToStringFn;
   ArgToStringCookie = nullptr;
 
   AllExtensionsSilenced = 0;
   IgnoreAllWarnings = false;
   WarningsAsErrors = false;
   EnableAllWarnings = false;
   ErrorsAsFatal = false;
   SuppressSystemWarnings = false;
   SuppressAllDiagnostics = false;
   ElideType = true;
   PrintTemplateTree = false;
   ShowColors = false;
   ShowOverloads = Ovl_All;
   ExtBehavior = diag::Severity::Ignored;
 
   ErrorLimit = 0;
   TemplateBacktraceLimit = 0;
   ConstexprBacktraceLimit = 0;
 
   Reset();
 }
 
-DiagnosticsEngine::~DiagnosticsEngine() {
-  if (OwnsDiagClient)
-    delete Client;
-}
-
 void DiagnosticsEngine::setClient(DiagnosticConsumer *client,
                                   bool ShouldOwnClient) {
-  if (OwnsDiagClient && Client)
-    delete Client;
-  
+  OwningClient.reset(ShouldOwnClient ? client : nullptr);
   Client = client;
-  OwnsDiagClient = ShouldOwnClient;
 }
 
 void DiagnosticsEngine::pushMappings(SourceLocation Loc) {
   DiagStateOnPushStack.push_back(GetCurDiagState());
 }
 
 bool DiagnosticsEngine::popMappings(SourceLocation Loc) {
   if (DiagStateOnPushStack.empty())

lib/Frontend/ASTUnit.cpp

 };
 
 /// \brief RAII object that optionally captures diagnostics, if
 /// there is no diagnostic client to capture them already.
 class CaptureDroppedDiagnostics {
   DiagnosticsEngine &Diags;
   StoredDiagnosticConsumer Client;
   DiagnosticConsumer *PreviousClient;
+  std::unique_ptr<DiagnosticConsumer> OwningPreviousClient;
 
 public:
   CaptureDroppedDiagnostics(bool RequestCapture, DiagnosticsEngine &Diags,
                           SmallVectorImpl<StoredDiagnostic> &StoredDiags)
     : 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());
   }
 };
 
 } // anonymous namespace
 
 void StoredDiagnosticConsumer::HandleDiagnostic(DiagnosticsEngine::Level Level,
                                               const Diagnostic &Info) {
   // Default implementation (Warnings/errors count).

lib/Frontend/CompilerInstance.cpp

static void SetUpDiagnosticLog(DiagnosticOptions *DiagOpts,
   }
 
   // Chain in the diagnostic client which will log the diagnostics.
   auto Logger = llvm::make_unique<LogDiagnosticPrinter>(*OS, DiagOpts,
                                                         std::move(StreamOwner));
   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,
                                        DiagnosticsEngine &Diags,
                                        StringRef OutputFile) {
   auto SerializedConsumer =
       clang::serialized_diags::create(OutputFile, 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)));
   }
 }
 
 void CompilerInstance::createDiagnostics(DiagnosticConsumer *Client,
                                          bool ShouldOwnClient) {

lib/Frontend/Rewrite/FixItRewriter.cpp

FixItRewriter::FixItRewriter(DiagnosticsEngine &Diags, SourceManager &SourceMgr,
                              const LangOptions &LangOpts,
                              FixItOptions *FixItOpts)
   : Diags(Diags),
     Editor(SourceMgr, LangOpts),
     Rewrite(SourceMgr, LangOpts),
     FixItOpts(FixItOpts),
     NumFailures(0),
     PrevDiagSilenced(false) {
-  OwnsClient = Diags.ownsClient();
-  Client = Diags.takeClient();
-  Diags.setClient(this);
+  OwningClient = Diags.takeClient();
+  Client = Diags.getClient();
+  Diags.setClient(this, false);
 }
 
 FixItRewriter::~FixItRewriter() {
-  Diags.takeClient();
-  Diags.setClient(Client, OwnsClient);
+  Diags.setClient(Client, !!OwningClient.release());
 }
 
 bool FixItRewriter::WriteFixedFile(FileID ID, raw_ostream &OS) {
   const RewriteBuffer *RewriteBuf = Rewrite.getRewriteBufferFor(ID);
   if (!RewriteBuf) return true;
   RewriteBuf->write(OS);
   OS.flush();
   return false;
void FixItRewriter::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
   Diag(Info.getLocation(), diag::note_fixit_applied);
 }
 
 /// \brief Emit a diagnostic via the adapted diagnostic client.
 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() {}

lib/Frontend/VerifyDiagnosticConsumer.cpp

 
 using namespace clang;
 typedef VerifyDiagnosticConsumer::Directive Directive;
 typedef VerifyDiagnosticConsumer::DirectiveList DirectiveList;
 typedef VerifyDiagnosticConsumer::ExpectedData ExpectedData;
 
 VerifyDiagnosticConsumer::VerifyDiagnosticConsumer(DiagnosticsEngine &Diags_)
   : Diags(Diags_),
-    PrimaryClient(Diags.getClient()), OwnsPrimaryClient(Diags.ownsClient()),
+    PrimaryClient(Diags.getClient()), OwningPrimaryClient(Diags.takeClient()),
     Buffer(new TextDiagnosticBuffer()), CurrentPreprocessor(nullptr),
     LangOpts(nullptr), SrcManager(nullptr), ActiveSourceFiles(0),
     Status(HasNoDirectives)
 {
-  Diags.takeClient();
   if (Diags.hasSourceManager())
     setSourceManager(Diags.getSourceManager());
 }
 
 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
 namespace {
 class VerifyFileTracker : public PPCallbacks {
   VerifyDiagnosticConsumer &Verify;
   SourceManager &SM;
 
if (PS == IsParsed) {
     UnparsedFiles.insert(std::make_pair(FID,
                                       UnparsedFileStatus(FE, FoundDirectives)));
   }
 #endif
 }
 
 void VerifyDiagnosticConsumer::CheckDiagnostics() {
   // Ensure any diagnostics go to the primary client.
-  bool OwnsCurClient = Diags.ownsClient();
-  DiagnosticConsumer *CurClient = Diags.takeClient();
+  std::unique_ptr<DiagnosticConsumer> OwningCurClient = Diags.takeClient();
+  DiagnosticConsumer *CurClient = Diags.getClient();
   Diags.setClient(PrimaryClient, false);
 
 #ifndef NDEBUG
   // In a debug build, scan through any files that may have been missed
   // during parsing and issue a fatal error if directives are contained
   // within these files.  If a fatal error occurs, this suggests that
   // this file is being parsed separately from the main file, in which
   // case consider moving the directives to the correct place, if this
if (SrcManager) {
     NumErrors += (PrintUnexpected(Diags, nullptr, Buffer->err_begin(),
                                   Buffer->err_end(), "error") +
                   PrintUnexpected(Diags, nullptr, Buffer->warn_begin(),
                                   Buffer->warn_end(), "warn") +
                   PrintUnexpected(Diags, nullptr, Buffer->note_begin(),
                                   Buffer->note_end(), "note"));
   }
 
-  Diags.takeClient();
-  Diags.setClient(CurClient, OwnsCurClient);
+  Diags.setClient(CurClient, !!OwningCurClient.release());
 
   // Reset the buffer, we have processed all the diagnostics in it.
   Buffer.reset(new TextDiagnosticBuffer());
   ED.Reset();
 }
 
 std::unique_ptr<Directive> Directive::create(bool RegexKind,
                                              SourceLocation DirectiveLoc,

tools/driver/driver.cpp

int main(int argc_, const char **argv_) {
 
   DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagClient);
 
   if (!DiagOpts->DiagnosticSerializationFile.empty()) {
     auto SerializedConsumer =
         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);
 
   Driver TheDriver(Path, llvm::sys::getDefaultTargetTriple(), Diags);
   SetInstallDir(argv, TheDriver);
 
   llvm::InitializeAllTargets();

unittests/Sema/ExternalSemaSourceTest.cpp

protected:
   virtual void ExecuteAction() {
     CompilerInstance &CI = getCompilerInstance();
     ASSERT_FALSE(CI.hasSema());
     CI.createSema(getTranslationUnitKind(), nullptr);
     ASSERT_TRUE(CI.hasDiagnostics());
     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);
     for (size_t I = 0, E = Sources.size(); I < E; ++I) {
       Sources[I]->InitializeSema(CI.getSema());
       CI.getSema().addExternalSource(Sources[I]);
     }
     ParseAST(CI.getSema(), CI.getFrontendOpts().ShowStats,