Problem copying information into pointer array

Question

I have a project in which I created an abstract class that represents a shape. I have a circle and a quad inherited from a shape and square inherited from a quad. Finally I have a class called allShapes that has a polymorphic array of Shape ** pointers and its size.

I need to implement the + operator, which receives an allShapes object and returns a new allShape with all elements located at this and other.

When I copy the part of this, the copy is done correctly but when I copy the parts from other I think it does not copy because when the function is finished when it comes to destruction I jump to an error that I am trying to delete blank content. what did I do wrong?

allShapes allShapes::operator+(const allShapes & other) const 
{
    allShapes newS;
    newS._size = (this->getSize() + other.getSize());
    int k = 0;
    newS._arr = new Shape*[newS.getSize()];

    for (int i = 0; i < this->getSize(); i++)
    {
        newS._arr[i] = this->_arr[i];
    }

    for (int j = this->getSize(); j < newS.getSize(); j++)
    {
        newS._arr[j] = other._arr[k++]; //i think here is the problem
    }
    return newS;
}

edit: i add the others methods that someone asks:

 allShapes::allShapes(const allShapes & other) //copy constructor
 {
this->_size = other.getSize();
this->_arr = new Shape*[other.getSize()];
for (int i = 0; i < other.getSize(); i++)
{
    this->_arr[i] = other._arr[i];
}
  }


 allShapes::~allShapes()//destructor to all elements
{
    if (this->_arr != NULL)
{
    for (int i = 0; i < this->_size; i++)
    {
        delete this->_arr[i];
    }
    delete[] this->_arr;
}

}

  class allShapes {
  private:
   Shape ** _arr;
   int _size;

First of all `Shape **` is a pointer to pointer. Second, why don't you use `std::vector` to store all your shapes instead of introducing new class with internal array of pointers to pointer? — vahancho, May 08 '19 at 06:49
@vahancho The task was to create a new array and then return an object of allShapes. Is my copy form from other wrong? So what's wrong? — Eden, May 08 '19 at 06:51
Problem may come from returning your structure by value (allShapes may have a dtor, etc). But as we don't have [mcve] we can't say anything more precise. As is, that looks almost correct. — Jean-Baptiste Yunès, May 08 '19 at 06:54
@Eden Due to the `allShapes` is being returned by value, this question is a non-starter until you post `allShapes` class and implementation, especially the copy constructor, assignment operator, and destructor. If you failed to implement those 3 functions mentioned, or if those functions have bugs, then that is more than likely the reason for the issues you're seeing now. — PaulMcKenzie, May 08 '19 at 07:05
@Eden So where is the assignment operator? Read up on the [rule of 3](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) — PaulMcKenzie, May 08 '19 at 07:15
@PaulMcKenzie why i need build one if i have copy constructor? if i will build assignment opertor i will use it in the copy constuctor no? — Eden, May 08 '19 at 07:17
Your class is broken without an assignment operator, and whoever or whatever told you that you didn't need one is totally wrong. What do you think will happen if you did `allStates x; allStates y; allStates z; z = x + y;`? Remember that a copy constructs a new object from an existing object, and an assignment operator assigns from one existing object to another existing object. — PaulMcKenzie, May 08 '19 at 07:18
@PaulMcKenzie ok Okay I understand. Do I need to build it like the copy constructor? And do I need to have a assignment operator in each of the previous departments? Because I remember he was not inherited — Eden, May 08 '19 at 07:21
You should post the `allStates` class itself. The assignment operator is very simple using copy/swap if the copy constructor works and destructor works correctly. — PaulMcKenzie, May 08 '19 at 07:22
@Eden -- [See this](http://coliru.stacked-crooked.com/a/d3cdfeae4d45c710). You need to implement that function before going forward attempting to fix your `operator +`. This is the consequence of returning these types of objects by value -- you need to make sure that the copy semantics are working 100%. — PaulMcKenzie, May 08 '19 at 07:26
@PaulMcKenzie is it fine like this? http://coliru.stacked-crooked.com/a/828a806d39825091 — Eden, May 08 '19 at 07:32
@Eden No. Leave the copy constructor as-is. There was no need to change it. I gave you the way to write the assignment operator, and your method goes and changes functions that didn't need to be changed. — PaulMcKenzie, May 08 '19 at 07:34
In addition, your method now leaks memory in the assignment operator. The whole idea of the copy/swap method is to avoid all of these issues (memory leakage, exception safety). — PaulMcKenzie, May 08 '19 at 07:38
@PaulMcKenzie your Right. i try to change it like you said but i still has the same problem :/ — Eden, May 08 '19 at 07:39
The point is that you needed to have the correct copy semantics straightened out first, as this would go nowhere without this prerequisite. The next step now is to work with the `operator +` itself to fix the issue. — PaulMcKenzie, May 08 '19 at 07:45

score 0 · Answer 1 · answered May 08 '19 at 07:23

what did I do wrong?

You used Shape ** to denote ownership of multiple Shape-derived objects, and copy the pointers. Whichever allShapes object that is destroyed first invalidates all the Shape *s in the other copy.

There are two possibilities for making it hard to go wrong. Either each allShapes has it's own copy of each Shape it holds, or they all share ownership. This is best expressed via a collection of either std::unique_ptr<Shape> for the former, or std::shared_ptr<Shape> for the latter.

class allShapes {
  private:
   std::vector<std::shared_ptr<Shape>> data;
  public:
   allShapes operator+(const allShapes & other)
   {
    allShapes copy = *this;
    copy.data.insert(copy.data.end(), other.data.begin(), other.data.end());
    return copy;
   }
   // compiler generated special members are correct
};

Do you have subclasses of `Shape` that `allShapes` will hold? Does `Shape` currently have a method like `virtual Shape * clone()`? — Caleth, May 08 '19 at 07:30
yes all shape is a contrainer of the the other classes i used already — Eden, May 08 '19 at 07:32

Problem copying information into pointer array

1 Answers1