On Dec 6, 2012, at 2:54 PM, Matthieu Monrocq <[email protected]> wrote:
> > > On Thu, Dec 6, 2012 at 8:18 PM, Jordan Rose <[email protected]> wrote: > Nice! > > On Dec 6, 2012, at 11:09 , Benjamin Kramer <[email protected]> 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 > [email protected] > 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
