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

[Howard Hinnant]

On Dec 6, 2012, at 2:54 PM, Matthieu Monrocq <matthieu.monrocq at gmail.com> wrote:

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

My position on this issue has shifted slightly over the past year.

Self-move-assignment should not cause a crash, unless you're specifically trying to find places where self-move-assignment happens by putting an assert in.  One place that self-move-assignment can happen is with self-swap.  Personally I consider self-swap a performance bug.  However there is nothing illegal about it.

Strictly speaking self-move-assignment is legal and should not crash.  However, unlike copy assignment, there is absolutely no need for self-move-assignment to be a no-op.  If self-move-assignment changes an object from a valid-not-moved-from state, to a valid-moved-from state, that is perfectly fine.

Typically the only place self-move-assignment happens, which is inside of self-swap, the operation happens on a moved-from state, and is often a no-op, as I suspect it would be just glancing at PartialDiagnostic.

In summary, my advice on the subject is to not check for self-move-assignment, but do ensure the algorithm won't crash if you do.  Not crashing (not placing the object in an invalid state) is all self-move-assignment need do if you want to be C++11 conforming.

Otoh, if you want to monitor, catch, and weed out self-swaps or other accidental self-move-assignments, putting an assert in there is the best way to do that.

Howard