Refactoring issue
Last year we applied a small refactoring in a piece of code. The construct was a parent - child relationship with the child hold by unique_ptr. Simplified the code was somewhat as follows:
struct Parent { explicit Parent(bool bShow) : m_bShow(bShow) { m_ptr = std::make_unique<Child>(this); } std::unique_ptr<Child> m_ptr; bool m_bShow; }; struct Child { explicit Child(Parent* pParent) : m_bShow(pParent->m_bShow) { } bool m_bShow; };
Code above has a bidirectional dependency between parent and child. It will compile if one splits out in header and source files for parent and child.
The heap use of child seemed redundant so it was refactored to be hold by value:
struct Parent { explicit Parent(bool bShow) : m_child(this) , m_bShow(bShow) { } Child m_child; bool m_bShow; };
This refactoring lead though to a bug where sometimes things were shown and sometimes not. The bug only appeared in release mode under certain conditions. It turned out that the value of 'm_bShow' was using uninitialized memory. We accidentally created the first memory safety issue in years!
The problem is that the child is created before the parent is fully created and thereby using uninitialized memory. The solution is easy by reversing creation order:
struct Parent { explicit Parent(bool bShow) : m_bShow(bShow) , m_child(this) { } bool m_bShow; Child m_child; };
Using the 'this' pointer in constructor is a code alarm but not a code smell. As a general rule here declare first all members of built in types before declaring members with classes. If that is not viable one can defer creation by using std::optional instead of std::unique_ptr. std::optional is often more optimal from a performance perspective.
Not sure how Rust would have prevented this; probably by disallowing the construct in the first place. That would be pity since circumventing the extra heap allocation and pointer access is certainly worthwhile the final solution where children are hold by value.
