[cfe-commits] r169535 - /cfe/trunk/include/clang/Basic/PartialDiagnostic.h

Matthieu Monrocq matthieu.monrocq at gmail.com
Thu Dec 6 11:54:04 PST 2012


On Thu, Dec 6, 2012 at 8:18 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> Nice!
>
> On Dec 6, 2012, at 11:09 , Benjamin Kramer <benny.kra at googlemail.com>
> wrote:
>
> > Author: d0k
> > Date: Thu Dec  6 13:09:30 2012
> > New Revision: 169535
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=169535&view=rev
> > Log:
> > Add move semantics to PartialDiagnostic, which can be very expensive to
> copy.
> >
> > Modified:
> >    cfe/trunk/include/clang/Basic/PartialDiagnostic.h
> >
> > Modified: cfe/trunk/include/clang/Basic/PartialDiagnostic.h
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/PartialDiagnostic.h?rev=169535&r1=169534&r2=169535&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/include/clang/Basic/PartialDiagnostic.h (original)
> > +++ cfe/trunk/include/clang/Basic/PartialDiagnostic.h Thu Dec  6
> 13:09:30 2012
> > @@ -19,6 +19,7 @@
> > #include "clang/Basic/Diagnostic.h"
> > #include "clang/Basic/SourceLocation.h"
> > #include "llvm/ADT/STLExtras.h"
> > +#include "llvm/Support/Compiler.h"
> > #include "llvm/Support/DataTypes.h"
> > #include <cassert>
> >
> > @@ -200,6 +201,14 @@
> >     }
> >   }
> >
> > +#if LLVM_HAS_RVALUE_REFERENCES
> > +  PartialDiagnostic(PartialDiagnostic &&Other)
> > +    : DiagID(Other.DiagID), DiagStorage(Other.DiagStorage),
> > +      Allocator(Other.Allocator) {
> > +    Other.DiagStorage = 0;
> > +  }
> > +#endif
> > +
> >   PartialDiagnostic(const PartialDiagnostic &Other, Storage *DiagStorage)
> >     : DiagID(Other.DiagID), DiagStorage(DiagStorage),
> >       Allocator(reinterpret_cast<StorageAllocator *>(~uintptr_t(0)))
> > @@ -242,6 +251,23 @@
> >     return *this;
> >   }
> >
> > +#if LLVM_HAS_RVALUE_REFERENCES
> > +  PartialDiagnostic &operator=(PartialDiagnostic &&Other) {
> > +    if (this != &Other) {
>
> Is this really necessary? If this is true, I think the move contract has
> been violated anyway.
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>

There was a discussion on stack overflow recently about this, and Howard
chimed in: it is unnecessary.

On the other hand, a `assert(this != &Other && "Illegal attempt to move
toward itself")` may make sense since this can quickly become a gotcha.

-- Matthieu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121206/345625fd/attachment.html>


More information about the cfe-commits mailing list