Operator + member function crashes about 1/8th of the time... implemented in terms of working member functions?

[mothergoose729]

I have a STRING class I have created that does more or less what you would expect a class called "STRING" to do. I have implemented operator+ in terms of my constructor and operator +=. About 1/8th of the time the program crashes, the rest of the time its fine. This suggest to me its a bad pointer but I haven't a clue where the problem is. If someone could help me out I would really appreciate it.

Some of the functions I haven't yet implemented, they are commented out in my implementation.

Head file

#ifndef _MYSTRING_H_ 
      #define _MYSTRING_H_ 

      #include <iostream>
      using namespace std;

      class STRING
      {
      private:
              
        char* _s; 
        unsigned _len; 
        
      public: 
        STRING(); //default constructor
        STRING(const char*);//constructor
        STRING(char); //constructor
        STRING(const STRING&);//copy constructor
        ~STRING(); //destructor
        
        int length() const;
        int position(char) const;
        void upcase(void);
        void downcase(void);
        void togglecase(void);
        
        char operator[] (unsigned) const;
        char& operator[] (unsigned);
        
        STRING& operator=(const STRING&);
        STRING& operator=(const char*);
        STRING& operator=(char);
        
        STRING& operator+=(const STRING&);
        STRING& operator+=(const char*);
        STRING& operator+=(char);
        
        STRING operator+ (const STRING& ) const;
        STRING operator+ (char*) const;
        STRING operator+(char) const;
        friend STRING operator+ (char*, const STRING&);
        friend STRING operator+ (char, const STRING&);
        
        bool operator== (const STRING& ) const; 
        bool operator== (char*) const; 
        friend bool operator== (char*, const STRING&); 
        
        bool operator< (const STRING& ) const; 
        bool operator< (char*) const; 
        friend bool operator< (char*, const STRING&); 
        
        bool operator!= (const STRING& ) const; 
        bool operator!= (char*) const; 
        friend bool operator!= (char*, const STRING&); 
        
        bool operator> (const STRING& ) const; 
        bool operator> (char*) const; 
        friend bool operator> (char*, const STRING&); 
        
        bool operator<= (const STRING& ) const; 
        bool operator<= (char*) const; 
        friend bool operator<= (char*, const STRING&); 
        
        bool operator>= (const STRING& ) const; 
        bool operator>= (char*) const; 
        friend bool operator>= (char*, const STRING&); 
        
        friend ostream& operator<< ( ostream&, const STRING&);
        friend istream& operator>> ( istream&, STRING&);
      };
      #endif 

Implementation file

#include "MyString.h"


STRING::STRING()
{
    _s = NULL;
    _len = 0;
}

STRING::STRING(const char* c)
{
    _len = 0;
    _s = NULL;
    if (c != NULL)
    {
        while ( c[_len] != '\0')
        {
            _len++;
        }
   
        _s = new char[_len];
   
        for (unsigned i = 0; i < _len; i++)
        {
        _s[i] = c[i];
        }
    }
    
}

STRING::STRING(char c)
{
   _len = 1;
   _s = new char[0];
   _s[0] = c;
}

STRING::STRING(const STRING& s)
{
    delete[] _s;
    _s = new char[s._len];
    _len = s._len;
    
    for (int i = 0; i < _len; i++)
    {
        _s[i] = s._s[i];
    }
}

STRING::~STRING()
{
    delete[] _s;
}
       
int STRING::length() const
{
    return _len;
}

int STRING::position(char inst) const
{
     for (int i = 0; i < _len; i++)
    {
        if (inst == _s[i])
        {
                 return i;
        }
    }
    
    return -1;
}

void STRING::upcase(void)
{    
    for(int i = 0; i < _len; i++)
    {
        if (_s[i] >= 'a' && _s[i] <= 'z')
        {
            _s[i] &= 223; 
        }
    }
}

void STRING::downcase(void)
{
    for(int i = 0; i < _len; i++)
    {
        if (_s[i] >= 'A' && _s[i] <= 'Z')
        {
            _s[i] |= 32; 
        }
    }
}

void STRING::togglecase(void)
{
    for(int i = 0; i < _len; i++)
    {
        if (_s[i] >= 'A' && _s[i] <= 'Z')
        {
            _s[i] |= 32; 
        }
        else if (_s[i] >= 'a' && _s[i] <= 'z')
        {
            _s[i] &= 223; 
        }
    }
}      
char STRING::operator[](unsigned index) const
{
    if (index >= _len)
    {
        cout << "Error, attempt to access outside of string";
        exit(200);
    }
    
    return _s[index];
}
char& STRING::operator[](unsigned index)
{
    if (index >= _len)
    {
        cout << "Error, attempt to access outside of string";
        exit(200);
    }
    
    return _s[index];
}   
    
STRING &STRING::operator=(const STRING& s)
{
    if (this == &s)
    {
        return *this;
    }
    
    delete[] _s;
    _s = new char[s._len];
    _len = s._len;
    
    for (int i = 0; i < _len; i++)
    {
        _s[i] = s._s[i];
    }
    
    return *this;
}    

STRING &STRING::operator=(const char* c)
{
     _len = 0;
     delete[] _s;
    _s = NULL;
    if (c != NULL)
    {
        while ( c[_len] != '\0')
        {
            _len++;
        }
   
        _s = new char[_len];
   
        for (unsigned i = 0; i < _len; i++)
        {
        _s[i] = c[i];
        }
    }
    
    return *this;
}

STRING &STRING::operator=(char c)
{
    _len = 1;
    delete[] _s;
   _s = new char[0];
   _s[0] = c;
    
    return *this;
}
           
STRING &STRING::operator+=(const STRING& s)
{   
    char* temp = new char[_len + s._len -1];
    int i = 0;
    
    for (; i < _len; i++)
    {
        temp[i] = _s[i];
    }
    
    for (int j = 0; j < s._len; j++)
    {
        temp[i+j] = s._s[j];
    }
    
    delete[] _s;
    _s = temp;
    _len+= s._len;
    
    return *this;
}

STRING &STRING::operator+=(const char* c)
{
    int charLen = 0;
    int i = 0;
    
     while ( c[charLen] != '\0')
        {
            charLen++;
        }
        
    char* temp = new char[_len + charLen -1];
    for (; i < _len; i++)
    {
        temp[i] = _s[i];
    }
    
    for (int j = 0; j < charLen; j++)
    {
        temp[i+j] = c[j];
    }
    
    delete[] _s;
    _s = temp;
    _len+= charLen;
    
    return *this;
}

STRING &STRING::operator+=(char c)
{
    
    int i = 0;
    char* temp = new char[_len];
    for (; i < _len; i++)
    {
        temp[i] = _s[i];
    }
    
    cout << c << endl;
    temp[i] = c;
    
    delete[] _s;
    _s = temp;
    _len++;
    
    return *this;
}
    

STRING STRING::operator+ (const STRING& s) const
{
       STRING temp = *this;
       temp+= s;
       return temp;
}

STRING STRING::operator+ (char* c) const
{
       STRING temp = *this;
       temp+= c;
       return temp;
}

STRING STRING::operator+ (char c) const
{
       STRING temp = *this;
       temp+= c;
       return temp;
}
     
STRING operator+ (char* c, const STRING& s)
{
      STRING temp(c);
      temp += s;
      return temp;
} 

STRING operator+ (char c, const STRING& s)
{
      STRING temp(c);
      temp += s;
      return temp;
} 
/*        
bool operator== (const STRING& ) const; 
bool operator== (char*) const; 
friend bool operator== (char*, const STRING&);         
bool operator< (const STRING& ) const; 
bool operator< (char*) const; 
friend bool operator< (char*, const STRING&);         
bool operator!= (const STRING& ) const; 
bool operator!= (char*) const; 
friend bool operator!= (char*, const STRING&);         
bool operator> (const STRING& ) const; 
bool operator> (char*) const; 
friend bool operator> (char*, const STRING&);         
bool operator<= (const STRING& ) const; 
bool operator<= (char*) const; 
friend bool operator<= (char*, const STRING&);         
bool operator>= (const STRING& ) const; 
bool operator>= (char*) const; 
friend bool operator>= (char*, const STRING&);         
*/
ostream& operator<< ( ostream& os, const STRING& s)
{
    for (int i = 0; i < s._len; i++)
    {
        os << s._s[i];
    }   
    
    os << '\0';   
} 
/*friend istream& operator>> ( istream& is, STRING&)
{
*/    

[mothergoose729]

My test main

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

using namespace std;

int main()
{
    STRING s;
    STRING s1 = "abcde";
    STRING s2 = "AbCdE";
    STRING s3 = "c";
    char c = 'a';
    cout << s1.length() << endl;
    cout << s1.position('c') << endl;
    cout << s1 << endl;
    cout << s3 << endl;
    s1.upcase();
    cout << s1 << endl;
    s1.downcase();
    cout << s1 << endl;
    cout << s2 << endl;
    s2.togglecase();
    cout << s2 << endl;
    s1 = s2;
    cout << s1 << endl;
    s1 = s2 = s3;
    cout << s1 << endl;
    s1 = c;
    cout << s1 << endl;
    s1 = "abcde";
    cout << s1 << endl;
    s2 = "fgh";
    s1 += s2;
    cout << s1 << endl;
    s1 = "abcde";
    s1 += "fgh";
    cout << s1 << endl;
    s1 += c;
    cout << s1 << endl;
    s3 += s3;
    //cout << s3 << endl;
    //s3 = s3 + s3;
    cout << s3 << endl;
    s3 = s3;
    cout << s3 << endl;
    //s3 = s3 + s3;
    //cout << s3 << endl;
    //s3 = s3 + "dd";
    //cout << s3 << endl;
    //s3 = "dd" + s3;
    //cout << s3 << endl;
    //s3 = s3 + 'a';
    //s3 = 'a' + s3;
    cin.get();
    return 0;
}

Here is my operators plus of type STRING + STRING function. This is the function giving my problems. I have looked to my operator= functions and operator+= functions for errors but have not been able to find any.

STRING STRING::operator+ (const STRING& s) const
{
       STRING temp = *this;
       temp+= s;
       return temp;
}

[ne555]

STRING::STRING(char c)
{
   _len = 1;
   _s = new char[0]; 
   _s[0] = c; //out of bounds (your array is zero size ¿?)
}

STRING::STRING(const STRING& s)
{
    delete[] _s; //¿what are you deleting?
    _s = new char[s._len];
    _len = s._len;
    
    for (int i = 0; i < _len; i++)
    {
        _s[i] = s._s[i];
    }
}

[vlad from moscow]

Your program is full of bugs. Let consider for example only two functions the default constructor and the operator +=( char c )

STRING::STRING()
{
    _s = NULL;
    _len = 0;
}

STRING &STRING::operator+=(char c)
{
    
    int i = 0;
    char* temp = new char[_len];
    for (; i < _len; i++)
    {
        temp[i] = _s[i];
    }
    
    cout << c << endl;
    temp[i] = c;
    
    delete[] _s;
    _s = temp;
    _len++;
    
    return *this;
}

Let assume that there is such code

STRING s;
s += 'A';

_len of the objetc s is equal to 0. And what are you doing in operator +=?

char* temp = new char[_len];

and after that

temp[i] = c;

You should be coution with pointers. Please check all your code.

Also it is a bad idea to assign names to variables that starts from undescore because such names reserved by the implementation.

[mik2718]

mystified by the -1 in the allocation, which looks like it will then be one character short

char* temp = new char[_len + s._len -1];

[Cubbi]

wow that's a lot of strange code

memory access errors:
1. in operator=: zero-length array allocation (new char[0]), followed by a write to the element number 1 (_s[0] = c;)
2. in operator+=: allocation for _len + s._len - 1, but the loop writes at the index i+j, which reaches _len + s._len - 1.
3. In the second operator+=, same story (allocation for _len + charLen - 1, accessing temp[_len + charLen - 1]
4. in the third operator+=, allocation for _len, but the last write (temp[i] = c, outside the loop) writes to temp[_len]

besides those errors, I see some compiler diagnostics:
1. comparison between signed and unsigned in 11 places
2. exit() called without a header file
3. operator<< lacks a return statement

and many style issues:
1. "using namespace std;" in a header file
2. binary operators implemented as member functions (even though they correctly call compound assignment operators internally)
3. constructors don't use member initializers
4. toupper/tolower replacements use magic numbers
5. other library code is reimplemented needlessly

PS: totally missed the delete _s; in the copy ctor

[vlad from moscow]

@Cubbi
1. "using namespace std;" in a header file

This complaint should be addressed to numerous authors of books on C++.:)

[mothergoose729]

Hi cubbi, thanks for your response.

1. That is strange yeah. I guess I could have just dynamically alocated a single character and set my pointer to it.
2. That is just a quirk in the implementation. The _len is the number of characters in the string, the first element is "1" not "0" so conversions can be odd at times. Not really related to my problem.
3. You have to create a temporary pointer to memory for that function. There isn't another way to do it.
4. I don't know whats strange about this. Temp = the contents of the object. The last character equals the content of the passed character. Again, there isn't a way to do this function without a temporary pointer.

1. I could be more careful about that.
2. My instructor is to blame for this one :p.
3. Thanks for point that out! Probably would have lost points on that. Fixed.

1. You can blame my instructor for this as well!
2. I don't know what you mean about this. Please elaborate.
3. I should do that. Need to brush up on Myers.
4. Again my instructor! I can't increment or decrement by a constant, but I can use unary operators with a constant. Strange specification but I have to follow it.
5. The purpose of this project is to re implement standard libraries.

PS: Please elaborate. As far as I know, dynamic memory is very neccessary in the copy constructor.

Cubbi, while I do appreciate your comments on my practice, suggestions for better alternatives would be very helpful.

Also, none of the things you mentioned are causing problems with my code, at least as far as I know. I would appreciate help finding exactly what is causing my errors. Thank you.

[Cubbi]

none of the things you mentioned are causing problems with my code

Huh? the first four were actual runtime errors, each of which was fatal under memory profiler (which is why I missed the stray delete in the copy ctor, it's never called. Once called, it crashed too).

The next batch were compiler diagnostics, I had to fix some of them to even get your code to compile.

The last batch was just things that stood out.

[mothergoose729]

I'm sorry I missunderstood you. I am going to take a close look at my code. I am not getting runtime errors with these, and my code never had trouble compiling, which has me confused. We are using a dated IDE though. Again, thank you.

[mothergoose729]

Would you mind explaining why some of these run time errors are occurring? I am not getting them in my testing (at least not yet) and I don't understand conceptually why they are causing problems. Thank you.

[clanmjc]

Your still having the issues? You've fixed the problems already stated? Ok, so where exactly is your program crashing, run the code over and over and give us the exact line in the code it breaks.

[Cubbi]

Would you mind explaining why some of these run time errors are occurring

Because dereferencing a pointer that's not pointing at an object is an error.
Take the last operator+= for example:

char* temp = new char[_len];

This created an array of _len char. its elements may be accessed as temp[0] to temp[_len-1], as you do in the following loop:

   for (; i < _len; i++)
    {
        temp[i] = _s[i];
    }

Now the loop ended because i == _len, and there you have

temp[i] = c;

There is no "temp[_len]", it doesn't exist.

Here's what I see:

 ABW: Array bounds write
      This is occurring while in:
            STRING::operator+=(char) [test.cc:311]
                   }
                   
                   cout << c << endl;
            =>     temp[i] = c;
                   
                   delete[] _s;
                   _s = temp;
            main           [test.cc:422]
            __start        [test.ibm-pure.tsk]
      Writing 1 byte to 0x30320f80 in the heap.
      Address 0x30320f80 is 1 byte past end of a malloc'd block at 0x30320f78 of 8 bytes.
      This block was allocated from:
...
            STRING::operator+=(char) [test.cc:304]
               {
                   
                   int i = 0;
            =>     char* temp = new char[_len];
                   for (; i < _len; i++)
                   {
                       temp[i] = _s[i];
            main           [test.cc:422]
            __start        [test.ibm-pure.tsk]

[mothergoose729]

EDIT: I'm an idiot. Thank you.

[mothergoose729]

I received the grade from my project today, I scored 98%! Thank you all for your help. Cubbi thanks for your patience with me, especially when I was being really dense.