-2

I want to build a copy constructor Pair(const Pair& other). This takes as its argument a read-only reference to another Pair. It should set up the newly constructed Pair as a "deep copy". But I have no idea on how to set up the integers at these new locations, which should be assigned values according to the integers that the other Pair is pointing to.

class Pair {
public:
  int *pa,*pb;
  Pair(int a, int b);
  Pair(const Pair & other);
  ~Pair();
};

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

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

Pair::~Pair(){
  delete pa；
  delete pb；
}

int main() {
  Pair p(15,16);
  Pair q(p);
  Pair *hp = new Pair(23,42);
  delete hp;

  std::cout << "If this message is printed,"
    << " at least the program hasn't crashed yet!\n"
    << "But you may want to print other diagnostic messages too." << std::endl;
  return 0;
}
robinzx117
  • 11
  • 1
  • 2
  • 1
    You're making it way more complicated than it needs to be. Your A and B members in the class should be int and not pointers to ints and you don't need to malloc those specifically. Also, assume that your object has already been allocated by the time you're inside your constructors and just copy the passed in values to your member variables. – Russ Schultz Oct 25 '19 at 00:21
  • 2
    `int *pa = new int;` in both of your constructors, is unrelated to the class member `pa`. Did you mean `pa = new int;`? Also, the `a = *pa;` is backwards, did you mean `*pa = a;`? The same for `pb`. In addition to that: you are leaking memory, since you never `delete` it. Consider learning from a [good C++ book](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list). – Algirdas Preidžius Oct 25 '19 at 00:22
  • 1
    @RussSchultz If one makes those variables as `int`s, instead of pointers to `int`s - the user defined copy constructor serves no purpose, since compiler-generated copy-constructor does everything one needs. – Algirdas Preidžius Oct 25 '19 at 00:24
  • @Algirdas Preidžius For the 'pointers to int', they are intended to point to heap memory locations that store integers. – robinzx117 Oct 25 '19 at 00:28
  • @robinzx117 Yes? Why do you think, I didn't know that? I wasn't even replying to you, with that comment. – Algirdas Preidžius Oct 25 '19 at 00:29
  • 2
    @robinzx117 You already have a copy constructor. Why do you think it doesn't do what you want to do? – eerorika Oct 25 '19 at 00:31
  • @eerorika He declares local variable so copy constructor do not copy values into member variables. – Phil1970 Oct 25 '19 at 00:40
  • Copy constructor's cool but [to finish things off](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) you should also have an assignment operator. Take a look at the [Copy and Swap Idiom](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) for a simple and safe way to do the assignment operator. It's not the fastest tool for the job, but it's damn near foolproof. – user4581301 Oct 25 '19 at 00:41
  • Another fun tool for the toolbox is the [Member Initializer List](https://en.cppreference.com/w/cpp/language/initializer_list). It'll come in handy later when you have complex member variables or base classes that lack default constructors. – user4581301 Oct 25 '19 at 00:48
  • @user4581301 While copy and swap idiom is often appropriate, in that case, it is overkill (and function might throw which is not the case in my answer). – Phil1970 Oct 25 '19 at 01:09

5 Answers5

3

Your first constructor could look like that:

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

And you don't need to write complex code multiple time by forwarding to the first constructor.

Pair::Pair(const Pair & other) 
    : Pair(*other.pa, *other.pb) 
{
}

Another thing is that you must also implement assignment operator. Otherwise, your code would be very error prone if you accidently do an assignment (as you would have a double delete assuming that your destructor is properly implemented.

Having said that, your destructor should be:

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

As other have said, it would be simpler to directly use int for members as you would not have to define copy and assignment yourself.

// Inside class declaration
Pair &operator=(const Pair &other);

// With other definitions.
Pair &Pair::operator=(const Pair &other)
{
    *pa = *other.pa;
    *pb = *other.pb;
    return *this;
}

If you really need pointers, then I would recommend you to use std::unique_ptr instead.

In your class the declaration become std::unique_ptr<int> pa; and similar for pb. At that point your destructor become empty. Remaining code could stay the same.

Also it is preferable to avoid making variable member public and even more in a case like this where you have dynamically allocated memory.

Phil1970
  • 2,605
  • 2
  • 14
  • 15
2

Your converting constructor is not assigning values to the ints it allocates, and it is not assigning those pointers to the class members.

Your copy constructor is likewise not assigning the allocated pointers to the class members. It is also not using the * operator correctly when accessing other's members.

Your destructor needs to delete the class members that the constructors allocated.

And you need to add a copy assignment operator to finish out the Rule of 3 properly.

Try this:

class Pair {
public:
  int *pa,*pb;
  Pair(int a, int b);
  Pair(const Pair & other);
  ~Pair();
  Pair& operator=(const Pair & other);
};

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

  /* alternatively:
  pa = new int(a);
  pb = new int(b);
  */
}

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

  /* alternatively:
  pa = new int(*(other.pa));
  pb = new int(*(other.pb));
  */
}

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

Pair& Pair::operator=(const Pair & other){
  *pa = *(other.pa);
  *pb = *(other.pb);
  return *this;
}

int main() {
  Pair p(15,16);
  Pair q(p);
  Pair *hp = new Pair(23,42);
  p = *hp;
  delete hp;

  std::cout << "If this message is printed,"
    << " at least the program hasn't crashed yet!\n"
    << "But you may want to print other diagnostic messages too." << std::endl;
  return 0;
}
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • 1
    For the assignment operator, if you use the `swap` idiom, then no need to verify for self assignment (that rarely occurs in practice). And in that case, swap idiom is not really appropriate as it make the code more complex. Ans if you want to use `swap` idiom, then it would make sense to define swap function for `Pair` objects. – Phil1970 Oct 25 '19 at 01:01
1

You can use custom constructor, copy constructor as below: 

 class Pair {
 public:
   int *pa,*pb;
   Pair(int, int);
   Pair(const Pair &);
  ~Pair();
 };

 /*
 * Implement its member functions below.
 */
 Pair::Pair(int a, int b){
  pa = new int;
  pb = new int;
  *pa = a;
  *pb = b;
}

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

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

 /* Here is a main() function you can use
  * to check your implementation of the
  * class Pair member functions.
  */

int main() {
  Pair p(15,16);
  Pair q(p);
  Pair *hp = new Pair(23,42);
  delete hp;

  std::cout << "If this message is printed,"
    << " at least the program hasn't crashed yet!\n"
    << "But you may want to print other diagnostic messages too." << std::endl;
  return 0;
}
bim
  • 612
  • 7
  • 18
0
class Pair { public: int *pa,*pb ; Pair(int, int); Pair(const Pair &); ~Pair(); };

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

Pair::Pair( const Pair &p) { Pair *pr=new Pair(*p.pa,*p.pb); pa = pr -> pa; pb = pr -> pb; } Pair::~Pair() { delete pa,pb; }

int main() { Pair p(15,16); Pair q(p); Pair *hp = new Pair(23,42); delete hp;

std::cout << "If this message is printed," << " at least the program hasn't crashed yet!\n" << "But you may want to print other diagnostic messages too." << std::endl; return 0; }
avariant
  • 2,234
  • 5
  • 25
  • 33
Minahil Ismail
  • 1
  • 1
-1

Your init constructor and copy constructor may have some mistakes.

the init constructor should be:

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

And copy constructor should be:

Pair::Pair(const Pair & other){
  pa = new int;
  pb = new int;
  *pa = *(other.pa);
  *pb = *(other.pb);
}
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
ming
  • 11