-1
class Pair {

public:

    int *pa,*pb;


    Pair(int a, int b) 
    {
        pa = new int(a);
        pb = new int(b);
    }

    Pair(const Pair& other) {
        int* pc = new int(*other.pa);
        int* pd = new int(*other.pb);
    }


    ~Pair() {
        delete pa;
        delete pb;
    }

};

In this program the compiler is producing a Segmentation fault(core dump) and after removing the destructor completely can we get the program to run without any errors so can anyone pls help me with this? Also even tho in the parameterized constructor I initialized the pointers the compiler gives warning that the point pa and pb are not initialized.

trincot
  • 317,000
  • 35
  • 244
  • 286
bhaveshgoel07
  • 19
  • 4
  • 1
    Possible duplicate of [What is The Rule of Three?](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) – Richard Critten Nov 03 '19 at 22:28
  • 2
    This code seems fine. You need to provide a minimal reproducible example of your errors (how you use the Pair objects) –  Nov 03 '19 at 22:29
  • 5
    In the copy constructor you don't initialise the member variables (you declare 2 new locals `pc` and `pd`) . Read the duplicate and please post a [mcve] which should include enough code to reproduce the problem. – Richard Critten Nov 03 '19 at 22:30
  • 3
    Your copy constructor is not setting the current instance's pa & pb. It is leaking/ immediately throwing away the memory it allocates. You are also missing your copy assignment operator. – Avi Berger Nov 03 '19 at 22:31
  • 1
    What are you doing here? ``` Pair(const Pair& other) { int* pc = new int(*other.pa); int* pd = new int(*other.pb); } ``` – Sebastian Nov 03 '19 at 22:33
  • why the gratuitous use of dynamic memory? – Caleth Nov 04 '19 at 14:24
  • actually the question called for it. – bhaveshgoel07 Nov 04 '19 at 22:37

4 Answers4

3

:Your copy constructor is creating two pointers and then just leaking them away. It never sets the member variables of the class.

You might want to refer to the rule of three / five / etc, and it wouldn't hurt to delete the default constructor for clarity.

class Pair {
public:
    int *pa, *pb;

    Pair() = delete;
    Pair(int a, int b): pa{new int{a}}, pb{new int{b}} {}
    Pair(const Pair& other): pa{new int{*other.pa}}, pb{new int{*other.pb} {}

    ~Pair(){
        delete pa;
        delete pb;
    }
};
Sebastian
  • 715
  • 6
  • 13
  • Ooops. That should have been `pa{new int{a}}` and `pb{new int{b}}`. Fixing. It sets `pa` to `new int{a}` and `pb` to `new int{b}`. – Sebastian Nov 04 '19 at 14:14
2

For your copy constructor, you should (most likely) be doing this:

Pair(const Pair& other) {
    pa = new int(*other.pa);
    pb = new int(*other.pb);
}

This does what you would expect a copy constructor to do.

Using your code, when you call your destructor on a Pair object made via its copy constructor, you are attempting to delete uninitialized pointers.

Adrian Mole
  • 49,934
  • 160
  • 51
  • 83
2

The likely problem (though it is hard to tell with only the code you have currently given) is these lines right here:

int* pc = new int(*other.pa);
int* pd = new int(*other.pb);

This re-declares new pointers pc and pd and does not actually set your member variables pa and pb. These new pointers go out of scope once the constructor exits. At the best this is a potential memory leak, at worst you are deleteing pointers that are uninitialized--which could be causing your crash.

To fix, you need to actually set your members and remove the int* keywords:

pa = new int(*other.pa);
pb = new int(*other.pb);
0

Instead of using raw pointers consider using std::unique_ptr for resource management:

#include <memory>

class Pair {
public:
    std::unique_ptr<int> pa, pb;

    Pair(int a, int b) 
    {
        pa = std::make_unique<int>(a);
        pb = std::make_unique<int>(b);
    }
};

int main() {
    Pair p {1, 2};
    return 0;
}
Håkon Hægland
  • 39,012
  • 21
  • 81
  • 174