Segfault in custom string class

Question

So I am trying to complete this very basic string class (MyString). Everything seemed to work, but when I uploaded it to the assignment site, it showed a segfault. The upload site uses electric fence, but it didn't give much insight as to where the fault occurred. It essentially runs through each function and returns a pass/fail/fault for it. In the case of the getline function, it returned a fault.

Also, the upload site uses valgrind which reported no errors.

EDIT: I almost forgot, when I called the function in the driver, it read from a file messages.txt, which contained one line of text: Testing this program... PLEASE WORK

Below is the getline function (as it exists in the implementation file) that appears to be the source of the fault:

// reads line from istream ... line end at newline char of choice) -- '\n' in this case
void MyString::getline(istream &inFile, char delimit)
{
    int index = 0;
    do
    {
        data[index] = inFile.get();
        index ++;
        if (index + 1 > capacity)
        {
            MyString tempStr;
            delete [] tempStr.data;
            tempStr.data = new char [capacity];
            for (int i = 0; i <= index; i++)
            {
                tempStr.data[i] = data[i];
            }
            capacity += 5;
            size = index;
            delete [] data;
            data = new char [capacity];
            for (int i = 0; i <= size; i++)
            {
                data[i] = tempStr.data[i];
            }
            delete [] tempStr.data;
            tempStr.data = NULL;
        }
    }
    while (!inFile.eof() && data[index-1] != delimit);
   if (data[index - 1] == delimit)
    {
        index -= 1;
        if (static_cast<double>(index)/capacity < .25 && capacity > 5)
        {
            capacity -= 5;
            char *temp = new char [capacity];
            for (int i = 0; i < index; i++)
            {
                temp[i] = data[i];
            }
            delete [] data;
            data = temp;
        }
    }
    data[index] = '\0';
    size = index + 1;
}

I feel like it's either something very simple I overlooked or a fundamental flaw in the way I approached this particular function. Any help is appreciated. I'm very new to programming (few weeks in) and am just trying to stay afloat -- got enrolled in CompSci 1 + 2 simultaneously.

Additionally, below is more of the implementation file -- particularly, the constructors (minus copy) and a few overloaded operators. While I could compile it on my end and concatenate class objects successfully, the upload site returned a fail when it tested "Concatenation." There wasn't any feedback as to which operator failed. I was curious what might cause that in my code. Thanks again.

#include <iostream>
#include <fstream>
#include "MyString.h"

using namespace std;

//default constructor - works
MyString::MyString()
{
    capacity = 5;
    size = 0;
    data = new char [capacity];
}

// constructor with character string
MyString::MyString(const char *cString) 
{
    int index = 0;
    capacity = 5;
    while ( cString[index] != '\0')
    {
        index++;
    }
    size = index + 1;
    while (size > capacity)
    {
        capacity += 5;
    }
    data = new char[capacity];
    for (int i = 0; i < size; i++)
    {
        data[i] = cString[i];
    }
}

// copy constructor
MyString::MyString(const MyString &aMyString)
{
    capacity = aMyString.capacity;
    size = aMyString.size;
    data = new char [capacity];
    for (int i = 0; i < size; i++)
    {
        data[i] = aMyString.data[i];
    }
}
// overloaded += operator
void MyString::operator+=(const MyString &aMyString)
{
    int tSize1 = size;
    int holder = 0;
    size += aMyString.size - 1;
    while (size > capacity)
    {
        capacity += 5;
    }
    char *tempArr = new char [capacity];
    for (int i = 0; i < (tSize1 - 1); i ++)
    {
        tempArr[i] = data[i];
    }
    for (int i = (tSize1 - 1); i < size; i++)
    {
        tempArr[i] = aMyString.data[holder];
        holder ++;
    }
    delete [] data;
    data = tempArr;
}

// overloaded + operator
MyString MyString::operator+(const MyString &aMyString) const
{
    int holder = 0;
    MyString tempS;
    int tSize1 = size + aMyString.size - 1;
    int tCap1 = capacity + aMyString.capacity;
    if (static_cast<double>(tSize1)/tCap1 < .25 && tCap1 > 5)
    {
        tCap1 -= 5;
    }
    tempS.size = tSize1;
    tempS.capacity = tCap1;
    delete [] tempS.data;
    tempS.data = new char [tempS.capacity];
    for (int i = 0; i < (size - 1); i ++)
    {
        tempS.data[i] = data[i];
    }
    for (int i = (size - 1); i < tSize1; i++)
    {
        tempS.data[i] = aMyString.data[holder];
        holder ++;
    }
    return tempS;
}

Answer 1

I don't know if these are all your bugs but I see two that stand out from the code:

for (int i = 0; i <= index; i++)
{
    tempStr.data[i] = data[i];
}
[snip]
for (int i = 0; i <= size; i++)
{
     data[i] = tempStr.data[i];
}

Both the for statements are accessing one more character than they should. If you want to process something 5 times for example in zero based indexing you check for i < 5 not i <= 5 . If I am not mistaken you have made that error in both these for loops. I believe they should be:

for (int i = 0; i < index; i++)

and

for (int i = 0; i < size; i++)

Writing memory beyond the edge of your arrays can cause problems like the segfaults.

Answer 2

Right away, the problem is your operator +=. It's supposed to return a reference to the object, not void.

Second, operator + should be written in terms of operator +=. Instead you wrote the whole operator + "from scratch", duplicating the code in operator +=

Here is how operator+ should be implemented:

// overloaded + operator
MyString MyString::operator+(const MyString &aMyString)
{
    MyString result = *this;   // copy the object
    result += aMyString;   // call the operator += (where the real work is done).
    return result;   // just return the result 
}

Your operator+=, needs to be defined as this:

MyString& MyString::operator+(const MyString &aMyString)
{
   // code to do work
   return *this;
}

Third, you failed to implement an assignment operator. You implemented the copy constructor, but not the assignment operator. Unless you didn't post it, it has to be implemented if you want to assign one string to another.

Last, your operator+= has a flaw. It changes the value of size here:

size += aMyString.size - 1;

Then attempts to allocate memory. What if that memory allocation fails (new throws an exception)? How do you "rollback" the value of size to its original value? You can't, at least not with your implementation.

To summarize, just because you have your implementation "working" doesn't mean it really is working properly. The things I pointed out above (no assignment operator, and operator += not returning a reference, *this) are just two issues.

The problem with assignments like this is that it can give you the false sense of accomplishment, when truthfully, what you have coded has bugs you never realized, but worse, can be easily created. For example:

int main()
{
  MyString s("abc");
  MyString t("123");
  s = t;
}

Without an assignment operator, that code fails due to a memory leak and a double deletion error.

It really takes an intermediate to advanced programmer to create yes, something that sounds simple as a "String" class properly, at least one that can pass all the tests that would make a String class usable in a real program.