[PATCH] Make DiagnosticsEngine::takeClient return std::unique_ptr<>

[Alexander Kornienko]

================
Comment at: include/clang/Basic/Diagnostic.h:370
@@ -370,3 +369,3 @@
   /// \brief Determine whether this \c DiagnosticsEngine object own its client.
-  bool ownsClient() const { return OwnsDiagClient; }
-  
+  bool ownsClient() const { return !!OwningClient; }
+
----------------
dblaikie wrote:
> 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.
Changed "!!" to "!= nullptr".

================
Comment at: include/clang/Rewrite/Frontend/FixItRewriter.h:69
@@ -68,3 +68,3 @@
   DiagnosticConsumer *Client;
-  bool OwnsClient;
+  std::unique_ptr<DiagnosticConsumer> OwningClient;
 
----------------
dblaikie wrote:
> 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).
Renamed Owning* to *Owner or just Owner depending on wether the latter is ambiguous in the context or not.

================
Comment at: lib/Frontend/Rewrite/FixItRewriter.cpp:45
@@ -44,4 +44,3 @@
 FixItRewriter::~FixItRewriter() {
-  Diags.takeClient();
-  Diags.setClient(Client, OwnsClient);
+  Diags.setClient(Client, !!OwningClient.release());
 }
----------------
dblaikie wrote:
> 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)
A separate scoped diagnostic client replacer would be nice, but it's slightly out of scope for this patch.

================
Comment at: lib/Frontend/VerifyDiagnosticConsumer.cpp:46
@@ -50,1 +45,3 @@
+  CheckDiagnostics();
+  Diags.takeClient().release();
 }
----------------
dblaikie wrote:
> While it didn't have it before - this might deserve a comment.
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.

http://reviews.llvm.org/D6294