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

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:143
+public:
+  MutableELFObject(ELFObjectFile<ELFT> &B)
+      : ELFObjectFile<ELFT>(std::move(B)),
----------------
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)


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

https://reviews.llvm.org/D64281