Not copying char arrays, function swap doesnt compile correctly and stringPtr is not modified

Question

//In header file: class definition:

class myString
{
public:

        myString(void);
     myString(const char *str);

     myString(const myString &); //copy constructor 
     ~myString(void); //destructor

     void swap(myString &from);


private:

     char *stringPtr;
     int stringLen;
};

//in cpp file, defining them member functions

myString::myString(const char *str)
{
    stringLen = strlen(str);

    stringPtr = new char[stringLen+1];

    strcpy(stringPtr,str);
    cout << "constructor with parameter called"<<endl;
}

myString::myString(const myString &str)
{

    stringPtr = new char[str.stringLen +1];
    strcpy(stringPtr,str.stringPtr);
    cout << "copyconstructor"<<endl;
}


void myString::swap(myString &from)
{
    myString buffer(from);
    int lengthBuffer = from.stringLen;

    from = new char[stringLen+1];
    from.stringLen = stringLen;
    strcpy(from.stringPtr, stringPtr);


    stringPtr = new char[lengthBuffer+1];
    stringLen = lengthBuffer;
    strcpy(stringPtr,buffer.stringPtr);
}

Answer 1

You can't modify a reference. Even if you replace it with a pointer modifying a pointer will not modify an object pointed to. Instead you need to work through the reference - just swap the fields.

void myString::swap(myString &from)
{
    std::swap( stringLen, from.stringLen );
    std::swap( stringPtr, from.stringPtr );
}

the above is using std::swap() as suggested by user sbi in comments. This is completely equivalent to the following (just for illustration, don't reinvent STL):

void myString::swap(myString &from)
    // First remember own length and pointer
    const int myOldLen = stringLen;
    char* myOldPtr = stringPtr;
    // now copy the length and pointer from that other string
    stringLen = from.stringLen;
    stringPtr = from.stringPtr;
    // copy remembered length and pointer to that other string
    from.StringLen = myOldLen;
    from.StringPtr = myOldPtr;
    // done swapping
}

Both will work even when called fro self-swapping:

myString string;
string.swap( string );

Answer 2

from = new char[stringLen+1]; should be from.stringPtr = new char[stringLen+1]; . Also remember to free the previously allocated memory before allocating new one.

Answer 3

Look closely at the line

from = new char[stringLen+1];

It is the same as

from = MyString(new char[stringLen+1]);

so your constructor of MyString get uninitialized array of chars. Then you trying to get the length of the string, but strlen just looping through chars of the string looking for 0 char. As we don't know what content uninitialized array of chars might have, we don't know what length strlen could return. It can even go further than array boundary and crash your program with segfault. But I can say for sure, after that there's not enough space in from.stringPtr to hold the string you want to copy in it.

So, use from.stringPtr = new char[stringLen+1]; or better from = MyString(*this); since you have copy constructor already.

Answer 4

You have already gotten a few good answers concerning the errors in you myString::swap() function. Yet, I'd like to add another one. There's some many things wrong with that function, I first found it hard to think of where to begin. But then I realized that you fail on some fundamental issue which I'd like to point out:

As a convention, a function called swap is expected to perform its task

1. in O(1)
2. without ever throwing an exception.

(Yes, I know, there are exceptions: std::tr1::array<>::swap(). But those should be very well justified.) Your implementation fails on both accounts. It is O(n) (strcpy) and might throw an exception (new) -- and it does so unnecessarily and without justification.

When you look at myString, you'll see that it only has two pieces of member data, which both are of built-in type. That means swapping two objects of this class is really simple to do while keeping to the conventions mentioned above: just swap the member data. That's as simple as calling std::swap on them:

void myString::swap(myString &from)
{
  std::swap(this->stringPtr,from.stringPtr);
  std::swap(this->stringLen,from.stringLen);
}

This is will never fail (swapping two pointers and two integers cannot fail), executes in O(1), is very easy to understand (well, once you get a grip on that swapping, anyway; it is an idiomatic form of implementing a class-specific swap function), and consists of two lines of code calling something well-tested in the standard library instead of 8 lines of code doing error-prone (and, in your case, erroneous) manual memory management.

Note 1: Once you've done this, you should specialize std::swap to call your implementation for your class:

namespace std { // only allowed for specializing function templates in the std lib
  template<>
  inline void std::swap<myString>(myString& lhs, myString& rhs)
  {
    lhs.swap(rhs);
  }

Note 2: The best (simple, exception-safe, and self-assignment-safe) way to implement assignment for your class is to use its swap:

myString& myString::operator=(const myString& rhs)
{
   myString tmp(rhs); // invoke copy ctor
   this->swap(tmp); // steal data from temp and leave it with our own old data
   return *this;
} // tmp will automatically be destroyed and takes our old data with it