[PATCH] D64281: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 1]

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 2 inline comments as done.
abrachet added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:143
+public:
+  MutableELFObject(ELFObjectFile<ELFT> &B)
+      : ELFObjectFile<ELFT>(std::move(B)),
----------------
labath wrote:
> abrachet wrote:
> > jhenderson wrote:
> > > abrachet wrote:
> > > > jhenderson wrote:
> > > > > Use `explicit` here. Should this be an r-value reference? What do you think?
> > > > Sounds fine to me. In the unit tests I move the ELFObjectFile but then continue to use it because as @rupprecht pointed out previously the move constructor is just a copy constructor. Is it safe to do this or should I explicitly copy?
> > > > Is it safe to do this or should I explicitly copy?
> > > 
> > > This doesn't sound safe to me - you need to copy or just simply not use the object after it's been moved. a) I wouldn't be surprised if this is technically undefined behaviour and the compiler makes assumptions about the state of the instance after the move. b) It's fragile to future changes.
> > > 
> > I did an explicit copy /I think/ in the unit test now. I have an `ELFObjectFile *` and then deference it and assign to an `ObjectFile &`. Does this make a copy or does the compiler omit it and just assign the address?
> Kind of the whole point of references is that they allow you to "refer" to other objects without making copies of them. If you want to make a copy, drop the `&` and assign it to an `ObjectFile` (or `ELFObjectFile`, or something)
Oh yeah. What am I talking about!


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

https://reviews.llvm.org/D64281