copy constructor for class with pointers as member variable

Question

i'm trying to write a copy constructor program for char pointer and int pointer .. i'm trying to copy content of object a1 to a2 and trying to display the values of a2.. the value of integer pointer is displayed correctly,but it is not printing the value pointed by char pointer,Please let me know what is the problem with this program

#include <iostream>
#include<string.h>

using namespace std;
class a
{
    private:
    char *sptr;
    int *i = new int;
    public:
    a()
    {

    }
    a(char str[])
    {

        sptr = new char[strlen(str) + 1];
        strcpy( sptr, str );
        *i=5;
    }
    a(const a& source)
    {
        cout<<"copy constructor"<<endl;
        sptr = new char[strlen(source.sptr) + 1];
        strcpy( sptr, source.sptr);
        i = source.i; 
    }

    ~a()
    {
        cout<<"destructor called"<<endl;
         delete sptr;
    }

    a operator=( const a& other )
    {
        cout<<"assigment overload"<<endl;
        if(&other!=NULL)
        {
            cout<<"inside assignment";
            a tmp( other );   //  Copy first, then swap.
            //std::swap( i, tmp.i );
            // std::swap( sptr, tmp.sptr );
            // strcpy( sptr, tmp.sptr);
        }
        return *this;     //  Dtor of tmp will clean up.
    }

void display()
    {

        // while(*sptr!='\0')
        // {
        //     cout<< *sptr;
        //     sptr++;
        // }
        // cout<<endl;
        // cout<<"i="<<*i<<endl;

        for ( char const* p = sptr; *p != '\0'; ++ p ) {
            cout << *p;
        }

    }
};
int main()
{
   // cout<<"contents of a1"<<endl;
   a a1("123456789898900");
   // a1.display();
   // a a2(a1);   
   // cout<<"contents of a2"<<endl;
   // a2.display();
   a a3;
   a3 = a1;
   // a3.display();
   return 0;
}

my modified program:

#include <iostream>
#include<string.h>

using namespace std;
class a
{
    private:
    char *sptr;
    // int *i = new int;
    public:
    a()
    {

    }
    a(char str[])
    {

        sptr = new char[strlen(str) + 1];
        strcpy( sptr, str );
        // *i=5;
    }
    a(const a& source)
    {
        cout<<"copy constructor"<<endl;
        sptr = new char[strlen(source.sptr) + 1];
        strcpy( sptr, source.sptr);
        // i = source.i; 
    }

    ~a()
    {

         delete sptr;
         cout<<"destructor called"<<endl;
    }

    a operator=( const a& other )
    {
        cout<<"assigment overload"<<endl;
        if(&other!=NULL)
        {
            cout<<"inside assignment";
            a tmp( other );   //  Copy constructor is called
            sptr = new char[strlen(tmp.sptr) + 1];
            strcpy( sptr, tmp.sptr);
        }//destructor is called once for tmp variable when it goes out of scope
        return *this;     //   Copy constructor is called,pass by value
    }//destructor is called once for this variable when it goes out of scope

void display()
    {

        // while(*sptr!='\0')
        // {
        //     cout<< *sptr;
        //     sptr++;
        // }
        // cout<<endl;
        // cout<<"i="<<*i<<endl;

        for ( char const* p = sptr; *p != '\0'; ++ p ) {
            cout << *p;
        }
        cout<<endl;

    }
};
int main()
{
   cout<<"contents of a1"<<endl;
   a a1("123456789898900");
   a1.display();
   a a2(a1);   //copy constructor called
   cout<<"contents of a2"<<endl;
   a2.display();
   a a3;
   a3 = a1;
   cout<<"contents of a3"<<endl;
   a3.display();
   return 0;
}//destructor called for a1,a2,a3


output:
contents of a1
123456789898900
copy constructor
contents of a2
123456789898900
assigment overload
inside assignmentcopy constructor
destructor called
copy constructor
destructor called
contents of a3
123456789898900
destructor called
destructor called
destructor called

Answer 1

The problem is that it's not at all clear what the semantics should be (nor why you're using int*). Do you want a deep copy (which is probably the only thing which makes sense given your first constructor); in that case, you need to implement the deep copy in the copy constructor; you also need to add an assignment operator and a destructor. (Without a destructor, you leak memory.) As for the concrete errors in what you do have:

sptr = (char*)malloc( sizeof( strlen( str ) ) );

• strlen returns a size_t, so sizeof( strlen( str ) ) is the same as sizeof( size_t ): probably either 4 or 8. You probably want just strlen( str ) + 1. (+ 1 because strlen doesn't count the final '\0'.)

• If this is C++, you would be much better off using new char[strlen(str) + 1], and avoiding the cast.

  sptr = str;

• You've now overwritten the pointer you got back from malloc, so you will inevitably leak memory. Also, the validity of sptr now depends on the lifetime of the argument; if it's a string literal, fine, but otherwise, you're likely to end up with a dangling pointer. What you probably wanted was strcpy( sptr, ptr );, to copy the actual characters.

And of course, I repeat the question: why is i a pointer, and not just an int?

In your copy constructor, you don't do a deep copy; if this is intentional, either you shouldn't be allocating memory in the other constructor, or you need some sort of reference counting. What you probably want is something more along the lines of:

A::A( A const& other )
    : i( other.i )      //  Assuming that `i` is `int`, and not `int*`
    : sptr( new char[strlen( other.sptr ) + 1] )
{
    strcpy( sptr, other.sptr );
}

And of course, you need an assignment operator and a destructor:

A& A::operator=( A const& other )
{
    A tmp( other ):   //  Copy first, then swap.
    std::swap( i, tmp.i );
    std::swap( sptr, tmp.sptr );
    return *this;     //  Dtor of tmp will clean up.
}

A::~A()
{
    delete sptr;
}

If i is really a pointer, you'll have to treat it similarly in the constructors and destructor.

Finally, in your display function, you're modifying the member pointer in the loop, which means that the next time around, you'll find it pointing to the '\0'. The output should be:

for ( char const* p = sptr; *sptr != '\0'; ++ p ) {
    std::cout << *p;
}

(I'm supposing here that you are doing this for pedantic reasons. If sptr points to a '\0' string, then std::cout << sptr << std::endl; is all you need in display.)

Answer 2

When invoking a1.display(), sptr gets increment to the end of the string.

In the copy constructor, sptr is copied to a2, it still points to the end of the string.

I would suggest using a local char pointer in your display function. This would fix the problem.