Shared pointer and copy constructor

[scaar]

Hello to everyone.
I would like to create a general function that is able to print some info that come from different kinds of objects.

There are at least two different situations: in the first one I receive a shared pointer (AddElementTypeA, AddElementTypeB), in the second one I receive an object and I want to create a shared pointer with that (AddElementTypeC).

In particular, in the second case I want to allocate a new area of memory, copy the content and set the shared pointer on that area. I thought the copy constructor could be useful in this sense, but my solution doesn't work.

Below you can find the code I wrote, can you help me to understand why it doesn't work?

class ObjA
{
	int temperature;
	std::string name;

public:
	ObjA(int t, std::string n)
	{
		temperature = t;
		name = n;
	}
};

class ObjB
{
	int volume;
	std::string name;

public:
	ObjB(int v, std::string n)
	{
		volume = v;
		name = n;
	}
};

class ObjC
{
	int distance;
	std::string name;

public:
	ObjC(int d, std::string n)
	{
		distance = d;
		name = n;
	}

	// copy constructor
	ObjC(const ObjC &v)
	{
		distance = v.distance;
		name = v.name;
	}
};

class ExportManager
{
	struct Data
	{
		char type;
		std::shared_ptr<void> data;
	};

	std::vector<Data> x;

	void AddElementTypeA(std::shared_ptr<ObjA> v)
	{
		Data y;
		y.type = 'A';
		y.data = std::static_pointer_cast<void>(v);

		x.push_back(y);
	}

	void AddElementTypeB(std::shared_ptr<ObjB> v)
	{
		Data y;
		y.type = 'B';
		y.data = std::static_pointer_cast<void>(v);

		x.push_back(y);
	}

	void AddElementTypeC(ObjC v)
	{
		Data y;
		y.type = 'C';

		y.data = std::make_shared<ObjC>(new ObjC(v));
	}

	void PrintElement()
	{
		if (!x.empty())
		{
			for (int i = 0; i < x.size(); i++)
			{
				switch (x[i].type)
				{
				case 'A':
				{
					std::shared_ptr<ObjA> v = std::static_pointer_cast<ObjA>(x[i].data);

					// do something with the shared pointer
					// ...

					break;
				}
				case 'B':
				{
					std::shared_ptr<ObjB> v = std::static_pointer_cast<ObjB>(x[i].data);

					// do something with the shared pointer
					// ...

					break;
				}
				case 'C':
				{
					std::shared_ptr<ObjC> v = std::static_pointer_cast<ObjC>(x[i].data);

					// do something with the shared pointer
					// ...

					break;
				}
				default:
					// throw an error
					break;
				}
			}

			x.clear();
		}
	}
};

The main issue is at row 80, where I try to create the shared pointer and call the copy constructor.

[coder777]

make_shared<...>(...) already creates the memory for you hence you don't need another new:

y.data = std::make_shared<ObjC>(v);

[scaar]

I'm new on C++ and shared pointer, if I write something like that

int foo()
{
    ObjC tmp;

    // set tmp data

    y.type = 'C';
    y.data = std::make_shared<ObjC>(tmp);
}

when I go out of the scope the tmp object is destroyed and the shared pointer (y.data) points to an invalid area, is that right?

[keskiverto]

No. https://www.cplusplus.com/reference/memory/make_shared/

  y.data = std::make_shared<ObjC>( tmp );
  // is same as:
  std::shared_ptr<ObjC> foo( new ObjC(tmp) );
  y.data = foo;

A new ObjC object is copy-constructed, initialized with value of 'tmp', in dynamically allocated memory.
A shared-pointer object is contructed and initialized the address of the new ObjC object.
Then you copy-assign the shared-pointer to y.data.

Therefore, the y.data points to object in dynamic memory, which does not die at end of the function, like the function-local automatic variable 'tmp'.

[dhayden]

Rather than having ExportManager::PrintElement() figure out what to do, add a virtual method to the objects that does it. That means each object should inherit from a base class.

Since each object has a name, it makes sense to put the name in the base class too.

In well designed software, each function or method does one thing. Also, your methods shouldn't "do the work." Instead they should "make doing the work easy." If you think this way then you'll end up with more flexible software.

For example, ExportManager::PrintElement() shouldn't delete x because printing and deleting are very different things. If the caller wants to print and delete, then they can do it in two steps.

Similarly, you should have just one Add() method that adds shared_ptr<Base>. If the caller has an object to add instead of a pointer, then they can make the shared pointer themselves.

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

class Base {
public:
    virtual ~Base() {};
    Base(const std::string &n) : name(n) {}
    std::string name;
    virtual void print() {};
};

class ObjA : public Base
{
    int temperature;

  public:
    virtual ~ObjA() {};
    ObjA(int t, std::string n) : Base(n), temperature(t)
    {
    }
    virtual void print() {
	std::cout << "A " << name << '\n';
    }
};

class ObjB : public Base
{
    int volume;

public:
    virtual ~ObjB() {};
    ObjB(int v, std::string n) : Base(n), volume(v)
    {
    }
    virtual void print() {
	std::cout << "B " << name << '\n';
    }
};

class ObjC : public Base
{
    int distance;

public:
    virtual ~ObjC() {};
    ObjC(int d, std::string n) : Base(n), distance(d)
    {
    }

    // copy constructor
    ObjC(const ObjC & v) : Base(v), distance(v.distance)
    {
    }
    virtual void print() {
	std::cout << "C " << name << '\n';
    }
};

class ExportManager
{
public:
    std::vector < std::shared_ptr<Base> > x;

    void Add(std::shared_ptr < Base > v)
    {
	  x.push_back(v);
    }
    void PrintElement()
    {
	for (unsigned i=0; i<x.size(); ++i) {
	    x[i]->print();
	}
    }
};


int main()
{
    std::shared_ptr<ObjA> pa(new ObjA(1, "Bob"));
    std::shared_ptr<ObjB> pb(new ObjB(1, "Ted"));
    ObjC c(3, "Alice");

    ExportManager xm;
    xm.Add(pa);
    xm.Add(pb);
    xm.Add(std::make_shared<ObjC>(c));
    xm.PrintElement();
}

$ ./foo
A Bob
B Ted
C Alice

[scaar]

Thanks for the suggestion, but unfortunately I cannot do that, because what I called ObjA, ObjB and ObjC are objects that I receive and they have nothing in common. Furthermore, I cannot change those objects so I cannot create a base class.

Because of that, I'm trying to use the std::static_pointer_cast<void>() solution.
The idea is to have a vector in which I can put inside different kinds of elements that have nothing in common. As you can imagine I cannot use the inheritance features.

Do you know if this is a good solution of if there are better and more safe?

[seeplus]

You could try using std::variant and then a vector of variant. Then use std::visit to process each element.

https://en.cppreference.com/w/cpp/utility/variant
https://en.cppreference.com/w/cpp/utility/variant/visit

[dhayden]

You could use multiple inheritance. Something like this with my original idea:

class Base {
    virtual void print();
};

class MyObjA : public Base, public ObjA {
    virtual void print();
};
class MyObjB : public Base, public ObjB {
    virtual void print();
};
class MyObjC : public Base, public ObjC {
    virtual void print();
};