When I learned C++ long ago, it was strongly emphasized to me that part of the point of C++ is that just like loops have "loop-invariants", classes also have invariants associated to the lifetime of the object -- things that should be true for as long the object is alive. Things that should be established by the constructors, and preserved by the methods. Encapsulation / access control is there to help you enforce the invariants. RAII is one thing that you can do with this idea.
Since C++11 we now have move semantics. For a class that supports moving, moving from an object does not formally end its life-time -- the move is supposed to leave it in some "valid" state.
In designing a class, is it bad practice if you design it so that the invariants of the class are only preserved up to the point that it is moved from? Or is that okay if it will allow you to make it go faster.
To make it concrete, suppose I have a non-copyable but moveable resource type like so:
class opaque {
opaque(const opaque &) = delete;
public:
opaque(opaque &&);
...
void mysterious();
void mysterious(int);
void mysterious(std::vector<std::string>);
};
And for whatever reason, I need to make a copyable wrapper for this object, so that it can be used, perhaps in some existing dispatch system.
class copyable_opaque {
std::shared_ptr<opaque> o_;
copyable_opaque() = delete;
public:
explicit copyable_opaque(opaque _o)
: o_(std::make_shared<opaque>(std::move(_o)))
{}
void operator()() { o_->mysterious(); }
void operator()(int i) { o_->mysterious(i); }
void operator()(std::vector<std::string> v) { o_->mysterious(v); }
};
In this copyable_opaque
object, an invariant of the class established at construction is that the member o_
always points to a valid object, since there is no default ctor, and the only ctor that isn't a copy ctor guarantees these. All of the operator()
methods assume that this invariant holds, and preserve it afterwards.
However, if the object is moved from, then o_
will then point to nothing. And after that point, calling any of the methods operator()
will cause UB / a crash.
If the object is never moved from, then the invariant will be preserved right up to the dtor call.
Let's suppose that hypothetically, I wrote this class, and months later, my imaginary coworker experienced UB because, in some complicated function where lots of these objects were being shuffled around for some reason, he moved from one of these things and later called one of its methods. Clearly it's his fault at the end of the day, but is this class "poorly designed?"
Thoughts:
It's usually bad form in C++ to create zombie objects that explode if you touch them.
If you can't construct some object, can't establish the invariants, then throw an exception from the ctor. If you can't preserve the invariants in some method, then signal an error somehow and roll-back. Should this be different for moved-from objects?Is it enough to just document "after this object has been moved from, it is illegal (UB) to do anything with it other than destroy it" in the header?
Is it better to continually assert that it is valid in each method call?
Like so:
class copyable_opaque {
std::shared_ptr<opaque> o_;
copyable_opaque() = delete;
public:
explicit copyable_opaque(opaque _o)
: o_(std::make_shared<opaque>(std::move(_o)))
{}
void operator()() { assert(o_); o_->mysterious(); }
void operator()(int i) { assert(o_); o_->mysterious(i); }
void operator()(std::vector<std::string> v) { assert(o_); o_->mysterious(v); }
};
The assertions don't substantially improve the behavior, and they cause a slow-down. If your project does use the "release build / debug build" scheme, rather than just always running with assertions, I guess this is more attractive, since you don't pay for the checks in the release build. If you don't actually have debug builds, this seems quite unattractive though.
- Is it better to make the class copyable, but not movable?
This also seems bad and causes a performance hit, but it solves the "invariant" issue in a straightforward way.
What would you consider to be the relevant "best practices" here?