Re: [PATCH] D12131: Make [Sema]DiagnosticBuilder move-only, instead of having a sneaky mutating copy ctor.

[David Blaikie via cfe-commits]

dblaikie updated this revision to Diff 32495.
dblaikie marked 2 inline comments as done.
dblaikie added a comment.

Addressed Richard's code review feedback


http://reviews.llvm.org/D12131

Files:
  include/clang/Basic/Diagnostic.h
  include/clang/Sema/Sema.h
  lib/ARCMigrate/TransformActions.cpp
  lib/Lex/LiteralSupport.cpp
  lib/Parse/Parser.cpp


Index: lib/Parse/Parser.cpp
===================================================================
--- lib/Parse/Parser.cpp
+++ lib/Parse/Parser.cpp
@@ -160,10 +160,15 @@
   if (EndLoc.isValid())
     Spelling = tok::getPunctuatorSpelling(ExpectedTok);
 
-  DiagnosticBuilder DB =
-      Spelling
-          ? Diag(EndLoc, DiagID) << FixItHint::CreateInsertion(EndLoc, Spelling)
-          : Diag(Tok, DiagID);
+  DiagnosticBuilder DB = [&]() {
+    if (!Spelling)
+      return Diag(Tok, DiagID);
+
+    auto D = Diag(EndLoc, DiagID);
+    D << FixItHint::CreateInsertion(EndLoc, Spelling);
+    return D;
+  }();
+
   if (DiagID == diag::err_expected)
     DB << ExpectedTok;
   else if (DiagID == diag::err_expected_after)
Index: lib/Lex/LiteralSupport.cpp
===================================================================
--- lib/Lex/LiteralSupport.cpp
+++ lib/Lex/LiteralSupport.cpp
@@ -69,8 +69,10 @@
   SourceLocation Begin =
     Lexer::AdvanceToTokenCharacter(TokLoc, TokRangeBegin - TokBegin,
                                    TokLoc.getManager(), Features);
-  return Diags->Report(Begin, DiagID) <<
-    MakeCharSourceRange(Features, TokLoc, TokBegin, TokRangeBegin, TokRangeEnd);
+  auto D = Diags->Report(Begin, DiagID);
+  D << MakeCharSourceRange(Features, TokLoc, TokBegin, TokRangeBegin,
+                           TokRangeEnd);
+  return D;
 }
 
 /// ProcessCharEscape - Parse a standard C escape sequence, which can occur in
Index: lib/ARCMigrate/TransformActions.cpp
===================================================================
--- lib/ARCMigrate/TransformActions.cpp
+++ lib/ARCMigrate/TransformActions.cpp
@@ -675,7 +675,9 @@
                                            SourceRange range) {
   assert(!static_cast<TransformActionsImpl *>(Impl)->isInTransaction() &&
          "Errors should be emitted out of a transaction");
-  return Diags.Report(loc, diagId) << range;
+  auto D = Diags.Report(loc, diagId);
+  D << range;
+  return D;
 }
 
 void TransformActions::reportError(StringRef message, SourceLocation loc,
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -1051,16 +1051,13 @@
     unsigned DiagID;
 
   public:
-    SemaDiagnosticBuilder(DiagnosticBuilder &DB, Sema &SemaRef, unsigned DiagID)
-      : DiagnosticBuilder(DB), SemaRef(SemaRef), DiagID(DiagID) { }
+    SemaDiagnosticBuilder(DiagnosticBuilder DB, Sema &SemaRef, unsigned DiagID)
+        : DiagnosticBuilder(std::move(DB)), SemaRef(SemaRef), DiagID(DiagID) {}
 
-    // This is a cunning lie. DiagnosticBuilder actually performs move
-    // construction in its copy constructor (but due to varied uses, it's not
-    // possible to conveniently express this as actual move construction). So
-    // the default copy ctor here is fine, because the base class disables the
-    // source anyway, so the user-defined ~SemaDiagnosticBuilder is a safe no-op
-    // in that case anwyay.
-    SemaDiagnosticBuilder(const SemaDiagnosticBuilder&) = default;
+    // DiagnosticBuilder's move ctor ensures that the source object becomes
+    // inactive, so ~SemaDiagnosticBuilder will be a safe/correct no-op on
+    // moved-from SemaDiagnosticBuilder instances.
+    SemaDiagnosticBuilder(SemaDiagnosticBuilder &&) = default;
 
     ~SemaDiagnosticBuilder() {
       // If we aren't active, there is nothing to do.
@@ -1090,12 +1087,18 @@
       BaseDiag << Value;
       return Diag;
     }
+    template <typename T>
+    friend SemaDiagnosticBuilder operator<<(SemaDiagnosticBuilder &&Diag,
+                                            const T &Value) {
+      const DiagnosticBuilder &BaseDiag = Diag;
+      BaseDiag << Value;
+      return std::move(Diag);
+    }
   };
 
   /// \brief Emit a diagnostic.
   SemaDiagnosticBuilder Diag(SourceLocation Loc, unsigned DiagID) {
-    DiagnosticBuilder DB = Diags.Report(Loc, DiagID);
-    return SemaDiagnosticBuilder(DB, *this, DiagID);
+    return SemaDiagnosticBuilder(Diags.Report(Loc, DiagID), *this, DiagID);
   }
 
   /// \brief Emit a partial diagnostic.
Index: include/clang/Basic/Diagnostic.h
===================================================================
--- include/clang/Basic/Diagnostic.h
+++ include/clang/Basic/Diagnostic.h
@@ -933,14 +933,12 @@
   }
   
 public:
-  /// Copy constructor.  When copied, this "takes" the diagnostic info from the
-  /// input and neuters it.
-  DiagnosticBuilder(const DiagnosticBuilder &D) {
-    DiagObj = D.DiagObj;
-    IsActive = D.IsActive;
-    IsForceEmit = D.IsForceEmit;
+  /// Move constructor. When moved, the source DiagnosticBuilder is made
+  /// inactive and will not emit a diagnostic during its destruction.
+  DiagnosticBuilder(DiagnosticBuilder &&D)
+      : DiagObj(D.DiagObj), NumArgs(D.NumArgs), IsActive(D.IsActive),
+        IsForceEmit(D.IsForceEmit) {
     D.Clear();
-    NumArgs = D.NumArgs;
   }
 
   /// \brief Retrieve an empty diagnostic builder.

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits