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

[dblaikie]

Summary

While there wasn't much use of "return Diag(...) << x" outside Sema (one
each in ARCMigrate, Lex, Parse, and 5 in ClangTidy - which were all just
changed to use named local variables, then stream, then return), in Sema
there were quite a few uses. So this extends Sema::Diag to be a variadic
template allowing the extra parameters to be passed directly rather than
via streaming on the resulting temporary expression.

[rsmith]

include/clang/Basic/Diagnostic.h

935–936

Comment is out of date.

include/clang/Sema/Sema.h

1082–1096

If we only need to support value category preservation for SemaDiagnosticBuilder, can we duplicate this template for the SemaDiagnosticBuilder &&operator<<(SemaDiagnosticBuilder &&Diag, const T &Value) case?

lib/Parse/Parser.cpp

163–172

This one might be more readable as

DiagnosticBuilder DB = std::move(Spelling ? ... : ...);

Thoughts?

[dblaikie]

Addressed Richard's code review feedback

include/clang/Basic/Diagnostic.h

935–936

Updated the wording a bit - the scare quotes and somewhat-vague "neuter" seemed unhelpful. Hopefully my rephrasing is an improvement, but I can restore the old or alternate wording if you prefer.

include/clang/Sema/Sema.h

1082–1096

Ah, good idea - that seems to totally work and removes the need for all the mechanical changes. (the handful of non-Sema diag cleanups in a few places (including clang-tidy) remain)

lib/Parse/Parser.cpp

163–172

Tried a few things here (move inside or outside the conditional operator) and none of it works. op<< takes its first arg by const ref (so that it can successfully take a temporary) and then returns it by const ref too - so we'd have to cast away constness before we could move from it.

Thoughts? Alternative approaches?

include/clang/Basic/Diagnostic.h

bool Emit() {
     bool Result = DiagObj->EmitCurrentDiagnostic(IsForceEmit);
 
     // This diagnostic is dead.
     Clear();
 
     return Result;
   }
   
 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.
   static DiagnosticBuilder getEmpty() {
     return DiagnosticBuilder();
   }
 
   /// \brief Emits the diagnostic.

include/clang/Sema/Sema.h

public:
   /// does) and, if the diagnostic comes from inside a template
   /// instantiation, printing the template instantiation stack as
   /// well.
   class SemaDiagnosticBuilder : public DiagnosticBuilder {
     Sema &SemaRef;
     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.
       if (!isActive()) return;
 
       // Otherwise, we need to emit the diagnostic. First flush the underlying
       // DiagnosticBuilder data, and clear the diagnostic builder itself so it
       // won't emit the diagnostic in its own destructor.
       //
       // This seems wasteful, in that as written the DiagnosticBuilder dtor will
       // do its own needless checks to see if the diagnostic needs to be
       // emitted. However, because we take care to ensure that the builder
       // objects never escape, a sufficiently smart compiler will be able to
       // eliminate that code.
       FlushCounts();
       Clear();
 
       // Dispatch to Sema to emit the diagnostic.
       SemaRef.EmitCurrentDiagnostic(DiagID);
     }
 
     /// Teach operator<< to produce an object of the correct type.
     template<typename T>
     friend const SemaDiagnosticBuilder &operator<<(
         const SemaDiagnosticBuilder &Diag, const T &Value) {
       const DiagnosticBuilder &BaseDiag = Diag;
       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.
   SemaDiagnosticBuilder Diag(SourceLocation Loc, const PartialDiagnostic& PD);
 
   /// \brief Build a partial diagnostic.
   PartialDiagnostic PDiag(unsigned DiagID = 0); // in SemaInternal.h
 

lib/ARCMigrate/TransformActions.cpp

 void TransformActions::applyRewrites(RewriteReceiver &receiver) {
   static_cast<TransformActionsImpl*>(Impl)->applyRewrites(receiver);
 }
 
 DiagnosticBuilder TransformActions::report(SourceLocation loc, unsigned diagId,
                                            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,
                                    SourceRange range) {
   report(loc, diag::err_mt_message, range) << message;
 }
 
 void TransformActions::reportWarning(StringRef message, SourceLocation loc,
                                      SourceRange range) {
   report(loc, diag::warn_mt_message, range) << message;
 }
 
 void TransformActions::reportNote(StringRef message, SourceLocation loc,
                                   SourceRange range) {
   report(loc, diag::note_mt_message, range) << message;
 }

lib/Lex/LiteralSupport.cpp

 /// a substring of a spelling buffer for the token beginning at \p TokBegin.
 static DiagnosticBuilder Diag(DiagnosticsEngine *Diags,
                               const LangOptions &Features, FullSourceLoc TokLoc,
                               const char *TokBegin, const char *TokRangeBegin,
                               const char *TokRangeEnd, unsigned DiagID) {
   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
 /// either a character or a string literal.
 static unsigned ProcessCharEscape(const char *ThisTokBegin,
                                   const char *&ThisTokBuf,
                                   const char *ThisTokEnd, bool &HadError,
                                   FullSourceLoc Loc, unsigned CharWidth,

lib/Parse/Parser.cpp

if (IsCommonTypo(ExpectedTok, Tok)) {
     return false;
   }
 
   SourceLocation EndLoc = PP.getLocForEndOfToken(PrevTokLocation);
   const char *Spelling = nullptr;
   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)
     DB << Msg << ExpectedTok;
   else
     DB << Msg;
 
   return true;
 }