Search code examples
c++referencemove-semanticsmember

Is it bad practice to reference class member object from within another member of the same class?


I'm rather new to C++ (with Java background) and I'm trying to understand the principles and practices for class designing.

From what I've already read it seems to me that one should prefer having class members as objects to having them as pointers (or references). - Please correct me if this is somehow wrong.

Based on that here's my design (version 1):

class Outer {
    Some_type _member;
    Inner _inner;
};

class Inner {
    Some_type* _ptr_to_outer_member;
};

I am aware that the lifetime of referenced object (Outer._member) needs to be guaranteed to exceed the lifetime of referencing object (Inner) and its pointer _ptr_to_outer_member to avoid dangling pointer. This condition should be satisfied here by the fact that _inner is a composite of _outer and Outer._member is an object member therefore it is always initialized.

Now I certainly don't need Outer to be copyable. Even being movable is not necessarily required since it's not supposed to be moved after everything is set up - although it'd be handy to have it movable when constructing other objects. Anyway with this design when Outer is moved _inner._ptr_to_outer_member gets invalidated. - This can be prevented by disabling copy and move operations for Outer but it kind of feels like it's just working around a bad practice. There's a guideline which says to avoid storing references to stack allocated variables and this case seems to me like it could have similar reasons to be avoided.

An alternative I can see is to heap allocate _member like this (version 2):

class Outer {
    std::unique_ptr<Some_type> _member;
    Inner _inner;
};

class Inner {
    Some_type* _ptr_to_outer_member;
};

Outer is now movable and non-copyable which is what I want but it's for the cost of heap allocating _member which is against the guideline about preferring object members.

So it gets me to the question: Are there any guidelines on this? I mean something like to use pointer to object as a members when you want to share the object (as in design version 2) - this would probably make the most sense to me (unless I'm missing something). Or to just disable moving for such objects (as in design version 1) and deal with it as it's non-movable.

Or is there something fundamentally wrong with the examples?

Thanks in advance for any insights on this.


Solution

  • There are some guidelines, but they are generic, like use unique_ptr for owning pointers.

    For the rest the guideline is: keep it simple. How did it come to this hierarchy with such an inter-dependency? Could there be a better way to structure your application?

    Bidirectional references are possible but you need need to maintain them yourself, or make Outer non-copyable and non-movable:

    class Outer {
        Some_type member;
        std::unique_ptr<Inner> inner;
    public:
        Outer() {}
    
        Outer(Outer const& that) = delete; // makes Outer not copyable/movable
        Outer& operator=(Outer const& that) = delete;
    
        /* The only way to update inner, also sets the back-reference */
        void setInner(std::unique_ptr<Inner> ptr) noexcept {
            inner = std::move(ptr);
            if (inner) {
                inner->ptr_to_outer_member = &member;
            }
        }
    };
    

    If Outer is movable, it should actively maintain all back-references to itself to keep them in-sync.

    For example like this:

    class Some_type {
    };
    class Inner {
        Some_type* ptr_to_outer_member;
        friend class Outer; // so that Outer may reach the private ptr_to_outer_member
    };
    
    class Outer {
        Some_type member;
        std::unique_ptr<Inner> inner; // owning pointer to Inner
    public:
        Outer() noexcept {
        }
        Outer(Outer&& that) noexcept {
            *this = std::move(that);
        }
        Outer& operator=(Outer&& that) noexcept {
            member = std::move(that.member);
            setInner(std::move(that.inner));
            return *this;
        }
        /* The only way to update inner, also sets the back-reference */
        void setInner(std::unique_ptr<Inner> ptr) noexcept {
            inner = std::move(ptr);
            if (inner) {
                inner->ptr_to_outer_member = &member;
            }
        }
    };
    
    int main() {
        Outer o;
        o.setInner(std::make_unique<Inner>());
        Outer o2(std::move(o)); // OK, move-construction
        Outer o3;
        o3 = std::move(o2); // OK, move-assignment
        Outer o4(o3); // error: Outer is not copyable
    }
    

    Having back-references in a movable type almost always requires a custom copy/move-constructor. Which means we have to also keep the rule of 3/5/0 in mind. In this example unique_ptr saves us from having a custom destructor. That may not always be the case.