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

[David Blaikie via cfe-commits]

dblaikie added inline comments.

================
Comment at: include/clang/Basic/Diagnostic.h:936-937
@@ -935,3 +935,4 @@
 public:
   /// Copy constructor.  When copied, this "takes" the diagnostic info from the
   /// input and neuters it.
+  DiagnosticBuilder(DiagnosticBuilder &&D) {
----------------
rsmith wrote:
> Comment is out of date.
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.

================
Comment at: include/clang/Sema/Sema.h:1085-1092
@@ -1084,10 +1084,10 @@
 
     /// 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;
     }
   };
----------------
rsmith wrote:
> 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?
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)

================
Comment at: lib/Parse/Parser.cpp:163-170
@@ -162,5 +162,10 @@
 
-  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;
+  }();
+
----------------
rsmith wrote:
> This one might be more readable as
> 
>   DiagnosticBuilder DB = std::move(Spelling ? ... : ...);
> 
> Thoughts?
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?


http://reviews.llvm.org/D12131