While reviewing some testing code, I have come across this situation:
Suppose we have these two classes:
class Project
{
public:
Project(std::string path);
~Project;
// [many more functions]
};
class ResourceManager
{
public:
ResourceManager(Project &proj);
virtual ~ResourceManager();
// [many more functions]
// [notably no getProject()]
};
In the test code, there exists a 'fake' version of the ResourceManager:
class FakeResourceManager : public ResourceManager
{
std::unique_ptr<Project> project_;
Project* makeProject()
{
project_= std::make_unique<Project>("/some/path"); // oops, accessing non-initialized member
return project_.get();
};
public:
FakeResourceManager()
: ResourceManager(*makeProject())
{}
virtual ~FakeResourceManager(); // defaulted in cpp
void markResourceAsDirty(); // 'extension' of base functionality for testing
};
This of course doesn't work, since the project_
member is not initialized yet when makeProject
is called, which the address sanitizer catches at runtime.
I reworked this initialization code, but I am unsure if it's now free of any UB and does the expected thing:
class FakeResourceManager : public ResourceManager
{
std::unique_ptr<Project> project_;
FakeResourceManager(std::unique_ptr<Project> project)
: ResourceManager(*project.get())
, project_(std::move(project)) //(1)
{}
public:
FakeResourceManager()
: FakeResourceManager(std::make_unique<Project>("/some/path"))
{}
virtual ~FakeResourceManager(); // defaulted in cpp
void markResourceAsDirty(); // 'extension' of base functionality for testing
}
Points I considered:
std::move
(1) of the unique_ptr
should not invalidate the pointer previously returned by get()
. The moved-to member should correctly delete the Project on destruction.ResourceManager
destructor uses the passed project reference it would be UB, since that object already got deleted by the derived class dtor. Since it doesn't, it should be ok to have a dangling reference here (or is this still UB?)Does this solution still contain UB (or some other logic error), or is this safe?
*project.get()
is unsafe to me (and can just be *project
).
To fix your problem, I'd do this:
struct HasProject {
std::unique_ptr<Project> project_;
[[nodiscard]] Project& makeProject(std::string_view path) {
assert(!project_);
project_= std::make_unique<Project>(path);
return *project_;
};
};
class FakeResourceManager : private HasProject, public ResourceManager
{
public:
FakeResourceManager()
: ResourceManager(makeProject("/some/path"))
{}
virtual ~FakeResourceManager(); // defaulted in cpp
void markResourceAsDirty(); // 'extension' of base functionality for testing
};
we move the storage into an earlier than ResourceManager
base class, and that base class is fully initialized by the time we construct ResourceManager
.
We can handle failures of project construction within makeProject
(throwing or whatever), and don't have a method that takes a unique_ptr
but fails catastrophically on an nullptr
.