On Tue, Aug 18, 2015 at 8:28 PM, David Blaikie via cfe-commits <
cfe-commits@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@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
