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

[David Blaikie via cfe-commits]

On Tue, Aug 18, 2015 at 8:28 PM, David Blaikie via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

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

I suppose my original change could allow us to change the op<<'s to take by
non-const ref and return by non-const ref, thus making this code fixable
with std::move? Though that'd be even more invasive (but help remove the
oddity of << mutating const objects... might eventually be able to make
DiagnosticBuilder's members non-mutable... )


>
> Thoughts? Alternative approaches?
>
>
> http://reviews.llvm.org/D12131
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150818/1132856d/attachment.html>