c++ passing pointer from constructor to constructor

Question

I'm working on learning C++ and I've created 3 classes: NodeEditor, Node, NodeIO.

Essentially the Editor contains a vector of Nodes, each of the Nodes has a vector of NodeIO instances.

I want each class to be able to reference there "owner".

Basically the NodeIO constructor takes a pointer from the Node its in, the Node takes a pointer to the Editor its in.

class NodeEditor {
    NodeEditor() {
        ...push_back(Node(this));
    }
}

class Node {
    NodeEditor* owner;
    Node(NodeEditor* _owner) : owner{ _owner } {
        ...push_back(NodeIO(this));
    }
}

class NodeIO {
    Node* owner;
    NodeIO(Node* _owner) : owner{ _owner } { }
}

I then need to use the _owner pointers later on.

When I attempt this in my project, at first the _owner pointer is pointing to the correct location, but once I need to retrieve it later, the actual object no longer exists at that pointers location.

What are my options to allow this sort of layout to work? And is there a different more recommended pattern to follow in this situation.

The code (as posted) looks like it should work fine; there must be some other reason why your pointers aren't getting maintained properly. If your classes have explicit copy-constructors and/or assignment-operators, you'll want to verify that they are copying the owner member-variables over to their target-objects. — Jeremy Friesner
– Jeremy Friesner, Commented May 26, 2018 at 21:48
You are doing everything right in the code that you posted. The error that creates a dangling pointer is somewhere else. The most likely reason I could think about is passing a pointer to "stack" object. Run valgrind or any other memory profiler to find out what is going on. — Sergey Kalinichenko
– Sergey Kalinichenko, Commented May 26, 2018 at 21:49
Remember that e.g. if you make a naive copy of a Node object, then the NodeIO sub-objects that the newly-created Node object holds will still have owner member-variables pointing at the old Node object, which is probably not what you want. (And remember also that data structures like std::vector will make copies of all the objects they hold when they need to resize their internal array larger) — Jeremy Friesner
– Jeremy Friesner, Commented May 26, 2018 at 21:49
Hmm thank's for the info, could it be that when I push the object into the vector via the push_back() operation, it kills the pointer location? — Hex Crown
– Hex Crown, Commented May 26, 2018 at 21:52
When you add items to a std::vector it will sometimes have to re-allocate it's content to keep it contigous in memory. If that happens pointers and references to those objects will become invalidated. Try using a std::deque instead and see it that works for you. A std::deque never re-allocates it's content. — super
– super, Commented May 26, 2018 at 21:54

R Sahu · Accepted Answer · 2018-05-26 23:01:27Z

You haven't shown any copy constructors. By that, I assume that you are relying on the default copy constructors provided by the compiler. That is the source of your problem.

When you use:

    ...push_back(Node(this));

in NodeEditor, you are storing a copy of Node(this). However, if Node and NodeIO don't have properly implemented copy constructors, the NodeIO objects in the Node object in the std::vector will point to a Node object that is not valid -- the temporary Node object.

Here's a sample program that shows the problem.

#include <iostream>
#include <vector>

struct Node;

struct NodeIO {
   Node* owner;
   NodeIO(Node* _owner) : owner{ _owner } { }
};

struct NodeEditor;

struct Node {
   NodeEditor* owner;
   Node(NodeEditor* _owner) : owner(_owner)
   {
      std::cout << (void*)this << std::endl;
      nodeIOList.push_back(NodeIO(this));
      nodeIOList.push_back(NodeIO(this));
   }

   std::vector<NodeIO> nodeIOList;
};

struct NodeEditor {
   NodeEditor()
   {
      nodeList.push_back(Node(this));
      nodeList.push_back(Node(this));
   }
   std::vector<Node> nodeList;
};

int main()
{
   NodeEditor editor;
   for ( auto& node : editor.nodeList )
   {
      std::cout << (void*)(&node) << std::endl;
      for (auto& nodeIO : node.nodeIOList )
      {
         std::cout << (void*)(nodeIO.owner) << std::endl;
      }
   }
}

Output:

0x7ffe53d34c30
0x7ffe53d34c50
0xae10c0
0x7ffe1af7a2a0
0x7ffe1af7a2a0
0xae10e0
0x7ffe1af7a2c0
0x7ffe1af7a2c0

The output clearly shows the values of the pointers to the Node objects that were constructed using Node(this) and the values of the pointers to the Node objects that are stores in the std::vector<Node>. Please note that the NodeIO objects still point to the temporary Node objects. They are dangling pointers in main.

I tried a quick fix but that did not work. I need to work on that a little bit more.

Here's a solution that works with default copy constructors. It uses std::vector of std::shared_ptrs instead of std::vector of objects.

#include <iostream>
#include <vector>
#include <memory>

struct Node;

struct NodeIO {
   Node* owner;
   NodeIO(Node* _owner) : owner{ _owner } { }
};

struct NodeEditor;

struct Node {
   NodeEditor* owner;
   Node(NodeEditor* _owner) : owner(_owner)
   {
      std::cout << (void*)this << std::endl;
      nodeIOList.push_back(std::make_shared<NodeIO>(this));
      nodeIOList.push_back(std::make_shared<NodeIO>(this));
   }

   std::vector<std::shared_ptr<NodeIO>> nodeIOList;
};

struct NodeEditor {
   NodeEditor()
   {
      nodeList.push_back(std::make_shared<Node>(this));
      nodeList.push_back(std::make_shared<Node>(this));
   }
   std::vector<std::shared_ptr<Node>> nodeList;
};

int main()
{
   NodeEditor editor;
   for ( auto& node : editor.nodeList )
   {
      std::cout << (void*)(node.get()) << std::endl;
      for (auto& nodeIO : node->nodeIOList )
      {
         std::cout << (void*)(nodeIO.get()->owner) << std::endl;
      }
   }
}

Output:

what would the internals of the copy constructor have to look like to make the pointers point to the proper non-temp locations?

Daniel H · Accepted Answer · 2018-05-26 22:14:44Z

1

When you add things to a vector, it is possible that all the elements of the vector move. Thus, when you add a Node, it is possible that all the existing NodeIOs' owner pointers are invalidated.

To handle this, you need to either

Use a different data structure and disable the Nodes copy and move constructors, or
During Node's move constructor, update the owner pointers of all the NodeIOs it contains, and make sure the new NodeIOs are constructed correctly if you call the Node copy constructor.

Depending on how you store NodeEditors, you should probably do the same with them.

answered May 26, 2018 at 22:14

Daniel H

7,5132 gold badges32 silver badges45 bronze badges

Comments

Ali Asadpoor · Accepted Answer · 2018-05-26 22:07:40Z

0

Check life time of NodeEditor object and Node Objects and NodeIo objects. Use new operator to create dynamic objects, or make sure your instantiation of NodeEditor and Node and NodeIo happens in the same scope. Check if you keep a copy of Node container of NodeEditor Object and you use it after releasing NodeEditor Object.

edited May 26, 2018 at 22:07

answered May 26, 2018 at 21:52

Ali Asadpoor

3271 silver badge13 bronze badges

Collectives™ on Stack Overflow

c++ passing pointer from constructor to constructor

3 Answers 3

1 Comment

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related