0

In the following code snippet, the Line destructor gets called twice, causing a double free issue. I have 2 questions here:

  1. I noticed that changing the code to: Line pCopy = pLine->deep_copy(); fixes the issue, but I'm still not clear what causes the difference.
  2. If I implement a copy constructor in Line class, and using the fix explained in "1", the copy constructor gets called instead of the desired deep_copy() function. What is the reason for this behavior and are there any workarounds?
struct Point
{
  int x{ 0 }, y{ 0 };

  Point(){}
  
  Point(const int x, const int y) : x{x}, y{y} {}
};

struct Line
{
  Point *start, *end;

  Line()
  : start{nullptr}, end{nullptr}
  {

  }

  Line(Point* const start, Point* const end)
    : start(start), end(end)
  {
  }

  ~Line()
  {
    delete start;
    delete end;
  }

  Line deep_copy() const
  {
    Point* p1 = new Point{*start};
    Point* p2 = new Point{*end};
    return { p1, p2 };
  }
};

int main()
{
    Point* start = new Point{1,2};
    Point* end = new Point{1,2};

    Line* pLine = new Line{start, end};
    Line pCopy;
    pCopy = pLine->deep_copy();
    pCopy.start->x = 3;
    pCopy.start->y = 4;
}

Edit: Please assume that the Line class has to store pointers to Point objects instead of storing the entire object inside it.

Hossam Elsamanoudy
  • 41
  • 1
  • 7
  • Your class does not have a custom copy constructor or assignment operator, and the implicitly created one is wrong for what your class does – UnholySheep Jul 30 '22 at 22:50
  • 2
    Nothing in your code requires `new`/`delete` or pointers. You can simply have objects of type `Point` or `Line` everywhere where you are using pointers to these types instead. And then you wouldn't need to bother about the rule-of-three and assignment would automatically deep copy. – user17732522 Jul 30 '22 at 22:54
  • `Point* start = new Point{1,2};` --> `Point start(1,2);`. The same with the other line that does this. In C++, `new` is not required to create objects. – PaulMcKenzie Jul 30 '22 at 23:12
  • *are there any workarounds?* -- As mentioned, [there is no need for new or delete](https://godbolt.org/z/x8PT1M4j4). All you did by introducing pointers is to make the code harder to maintain, as well as *less* efficient, due to the compiler not having a chance to implement optimal defaults for copy and destruction. – PaulMcKenzie Jul 30 '22 at 23:19
  • @UnholySheep Exactly. What I'm saying is that when I explicitly create a copy constructor that does the deep copy for the Line class, it gets called instead of the deep_copy() function as explained in the question. – Hossam Elsamanoudy Jul 30 '22 at 23:23
  • 1
    `pCopy = pLine->deep_copy();`: This line is using the copy assignment operator. That is what `=` does. You can't have the copy assignment operator shallow copy (the default if you don't implement it yourself) because then you end up with two objects owning the allocated memory and trying to delete it. You need to define copy constructor and copy assignment operator to perform a deep copy and then you won't need `deep_copy` at all. (The alternative is to delete the copy assignment and constructor and define move operations instead which zero the pointers of the source properly.) – user17732522 Jul 30 '22 at 23:47
  • Also, if you for some reason need pointers instead of the objects themselves inside `Line`, you can simply use `std::unique_ptr` instead, which will make sure the alternative I mentioned above is automatically implemented by the compiler. You can then define your `deep_copy` if you want to without needing any constructor, assignment or destructor definitions. – user17732522 Jul 30 '22 at 23:49
  • @user17732522 Many thanks. I used your suggestion to make use of the move operations and it worked fine. – Hossam Elsamanoudy Jul 31 '22 at 09:44

0 Answers0