[PATCH] D150020: Fix possible self assign issue for DIEValue

[Kan Shengchen via Phabricator via llvm-commits]

skan added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/DIE.h:469
+      return *this;
     destroyVal();
     Ty = X.Ty;
----------------
XinWang10 wrote:
> XinWang10 wrote:
> > skan wrote:
> > > https://stackoverflow.com/questions/12015156/what-is-wrong-with-checking-for-self-assignment-and-what-does-it-mean
> > > 
> > > Use copy-and-swap method?
> > > ```
> > > DIEValue &operator=(DIEValue X) {
> > >   swap(*this, X);
> > >   return *this;
> > > }
> > > ```
> > This reported a lot of errors to me, I think swap will also call operator assign? 
> > From my point of view, the logic here for assign is correct if we just remove destroyVal(), since the Val is on stack, we don't need to destroy it and it will be overwrite by X. But I'm not sure if there is some concern here.
> > 
> > When I read this code, another question occured to me, why do we need the explicit copy and assign constructor, this class don't maintain any dynamic memory. 
> > I think if I remove the copy and assign constructor, it will also work for me.
> > But I don't want to make that big change :)
> Seems can pass check-all locally without copy and assign construction function.
swap does not call assignment operator, and it swaps the members of two classes.
It seems `copyVal` will dynamically allocate memory.

What's the error?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150020/new/

https://reviews.llvm.org/D150020