[PATCH] D25645: [ADT] Add CachedHashString.

[Justin Lebar via llvm-commits]

jlebar added inline comments.


================
Comment at: llvm/include/llvm/ADT/CachedHashString.h:117
+  CachedHashString &operator=(const CachedHashString &Other) {
+    if (!isEmptyOrTombstone())
+      delete[] P;
----------------
dblaikie wrote:
> timshen wrote:
> > How do you feel about this:
> >   if (this != Other) {
> >     ~CachedHashString();
> >     new (this) CachedHashString(Other);
> >   }
> >   return *this;
> > 
> > Likewise for the move assign.
> You can potentially just use the unified copy/move op roughly:
> 
>   foo &operator=(foo f) {
>     swap(*this, f);
>   }
> 
> (you can actually implement the assignment here rather than use a custom swap - then rely on std::swap to be cheap enough - or the other way around, whichever's better/worth it)
> 
> & then you do need to implement copy and move ctors, but you've already got those.
I like this, because the swap function is simple to write.  It lets me avoid the problem of blowing up on

  CachedHashString x;
  x = x;

which I had before.
 
Thank you for the suggestion!


https://reviews.llvm.org/D25645