9

I have an object that is related to some file stored on the disk. The object's constructor accepts this file as an argument, reads it and creates an actual object with the settings depending on the file's content. During the runtime there is a probability for this file to be modified by the user. The object has a method that checks if the file has been modified since the last read, and if this is true, the object has to be updated. There are enough members to be updated inside the object, so instead of manually update each of them it is more convenient to simply create a new object and move it into the existing one(less typing and better readability). But is it actually safe to change *this during the method execution like in the example below?

void Object::update()
{
    if (this->isModified(file)) // Check if a file has been modified since the last read
    {
        try
        {
            Object newObject(file); // May throw
            *this = std::move(newObject); // Is it safe?
        }
        catch (const std::exception& ex)
        {
            // Invalidate and rethrow exception
            this->invalidate();
            throw(ex);
        }
    }
    // ...
}
Alexey104
  • 969
  • 1
  • 5
  • 17
  • @463035818_is_not_a_number: Alreadsy deleted... yes, I saw it in the example code... :-) – Klaus Dec 18 '21 at 09:59
  • 1
    Not sure but I think `newObject`'s scope ends after try block. And accessing out-of-scope object is UB. – TheScore Dec 18 '21 at 10:02
  • 2
    I think it would be more readable defining a `parse_file()` method used by constructor and `update()`. btw you don't need to explicitly refer to `this` in the member functions body. – MatG Dec 18 '21 at 10:02
  • 1
    Move `*this = std::move(newObject);` to inside the `try` block. – Richard Critten Dec 18 '21 at 10:04
  • @TheScore and Richard Critten, Thank you, you are right, edited. – Alexey104 Dec 18 '21 at 10:05

1 Answers1

8

You seem to be worried about this appearing on the left hand side, though *this = ... merely calls operator=. Typically the assignment operator that takes a rvalue reference just moves the members.

Consider this simpler example:

struct foo {
    int x = 42;
    foo& operator=(foo&& other){
        x = std::move(other.x);
        return *this;
    }
    void bar(){
        foo other;
        operator=(std::move(other));
    }
};

Nothing wrong with that.

Another way to convince yourself that *this = std::move(newObject); is not problematic is to inline all code from the assignment operator to update. For the example above that would be:

struct foo {
    int x = 42;
    void bar(){
        foo other;
        x = std::move(other.x);
    }
};
463035818_is_not_an_ai
  • 109,796
  • 11
  • 89
  • 185