Dynamic memory help!

[ETAkoG1T]

I have had some problems with dynamic memory in the past, what makes sense to me often proves to be wrong... The error is that on runtime a window pops up talking about corrupt memory or something and I have to abort the program. This part of the program seems fine to me but I suspect there is a problem in it:

string.h

#ifndef STRING1_H_
#define STRING1_H_
#include <iostream>
using std::ostream;
using std::istream;

class String
{
private:
	char * str;
	int len;
	static int num_strings;
	static const int CINLIM = 80;
public:
	//constructors and other methods
	String(const char * s);
	String();
	String(const String &); // copy constructor
	~String();
	int length () const { return len; };
	void stringlow();
	void stringup();
	int has(char ch);
	//overloaded operator methods
	String & operator=(const String &);
	String & operator=(const char *);
	char & operator[](int i);
	const char & operator[](int i) const;
	//overloaded operator friends
	friend String operator+(const String &st, const String &st2);
	friend bool operator<(const String &st, const String &st2);
	friend bool operator>(const String &st1, const String &st2);
	friend bool operator==(const String &st, const String &st2);
	friend ostream & operator<<(ostream & os, const String & st);
	friend istream & operator>>(istream & is, String & st);
	//static function
	static int HowMany();
};

#endif 

Main.cpp

// Ch12_2.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include <iostream>
#include "string1.h"
using namespace std;

int main()
{
	String s1(" and I am a C++ student.");
	String s2 = "Please enter your name: ";
	String s3;
	cout << s2;
	cin >> s3;
	s2 = "My name is " + s3;
	cout << s2 + s1;
	s2.stringup();
	cout << "The string\n" << s2 << "\ncontains " << s2.has('A')
		<< " 'A' characters in it.\n";
	s1 = "red";

	String rgb[3] = { String(s1), String("green"), String("blue")};
	cout << "Enter the name of a primary color for mixing light: ";
	String ans;
	bool success = false;
	while (cin >> ans)
	{
		ans.stringlow();
		for (int i = 0; i < 3; i++)
		{
			if(ans == rgb[i])
			{
				cout << "That's right!\n";
				success = true;
				break;
			}
		}
		if(success)
			break;
		else
			cout << "Try again\n";
	}
	return 0;
}

string.cpp

#include <cstring>
#include <iostream>
#include "string1.h"
#include <ctype.h>

using std::cin;
using std::cout;

// initializing static class member

int String::num_strings = 0;

//static method
int String::HowMany()
{
	return num_strings;
}

//class methods
String::String(const char * s)
{
	len = std::strlen(s);
	str = new char[len + 1];
	std::strcpy(str, s);
	num_strings++;
}

String::String()
{
	len = 1;
	str = new char[1];
	str[0] = '\0';
	num_strings++;
}

String::String( const String & st) 
{
	num_strings++;
	len = st.len;
	str = new char [len + 1];
	std::strcpy(str , st.str);
}

String::~String()
{
	--num_strings;
	delete [] str;
}

//overloaded operator methods
// assign a String to a String

String & String::operator=(const String & st)
{
	if( this == &st)
		return *this;
	delete [] str;
	len = st.len;
	str = new char[len + 1];
	std::strcpy(str, st.str);
	return *this;
}

//assign a c string to a string
String & String::operator=(const char * s)
{
	delete [] str;
	len = std::strlen(s);
	str = new char[len + 1];
	std::strcpy(str, s);
	return *this;
}

char & String::operator[](int i)
{
	return str[i];
}

//read-only char access for const String
const char & String::operator[](int i) const
{
	return str[i];
}

//overloaded operator friends

bool operator<(const String &st1, const String &st2)
{
	return (std::strcmp(st1.str, st2.str) < 0);
}

bool operator>(const String &st1, const String &st2)
{
	return st2 < st1;
}

bool operator==(const String &st1, const String &st2)
{
	return (std::strcmp(st1.str, st2.str) == 0);
}

ostream & operator<<(ostream & os, const String & st)
{
	os << st.str;
	return os;
}

istream & operator>>(istream & is, String & st)
{
	char temp[String::CINLIM];
	is.get(temp, String::CINLIM);
	if (is)
		st = temp;
	while (is && is.get() != '\n')
		continue;
	return is;
}


String operator+(const String &st, const String &st2)
{
	String str;
	str.str = new char[st.len + st2.len + 1];
	std::strcpy(str.str, st.str);
	std::strcpy(st.len + str.str, st2.str);
        str.len = std::strlen(str.str);

	return str;
}

void String::stringlow()
{
	int i = 0;
	while(str[i])
	{
		str[i] = tolower(str[i]);
                i++;
	}
}

void String::stringup()
{
	int i = 0;
	while(str[i])
	{
		str[i] = toupper(str[i]);
                i++;
	}
}

int String::has(char ch)
{
	int count = 0;
	int i = 0;
	while(str[i])
	{
		if(str[i] == ch)
			count++;
                i++;
	}
	return count;
}

[Peter87]

How have you implemented the copy constructor?

[ETAkoG1T]

String str3 = str1 + str2; // this should work making str3 equal to str1
// followed by tr2

Sorry I don't quite understand... Isn't '=' the copy constructor?

[Peter87]

Yes that will use the copy constructor. That is why you have to define the copy constructor to do what you want. The default copy constructor just copy each data member (len and str).

Also see the rule of three.
http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29

[ETAkoG1T]

str1 and str2 is from the String class I made, look at my original post. I defined the copy constructor! I might be wrong but I think I already posted it. What I need help with is finding out if they are compatible and if they are not find a solution. BTW this is a exercise from C++ Primer Plus were I have to implement the + operator.

[Peter87]

The operator= that you posted is the copy assignment operator and is used when copying an object into an already existing object. The copy constructor is a constructor and is used when an object is created to be a copy of another object.

String str1; // default constructor
String str2 = str1; // copy constructor
str1 = str2; // copy assignment operator 

[ETAkoG1T]

Actually when I look at my programs main function I noticed that the program never actually used the copy constructor only the copy assignment operator so the problem is in the copy assignment operator or the + operator.
Here is the main file:

#include <iostream>
#include "string1.h"
using namespace std;

int main()
{
	String s1(" and I am a C++ student.");
	String s2 = "Please enter your name: ";
	String s3;
	cout << s2;
	cin >> s3; // there is a break point somewere around here I think
	s2 = "My name is " + s3; // this is were it goes wrong with memory
	cout << s2 + s1; // or this
	s2.stringup(); // when it reaches this line it has already stopped and 
                            // an error message about memory pops up.
	cout << "The string\n" << s2 << "\ncontains " << s2.has('A')
		<< " 'A' characters in it.\n";
	s1 = "red";

	String rgb[3] = { String(s1), String("green"), String("blue")};
	cout << "Enter the name of a primary color for mixing light: ";
	String ans;
	bool success = false;
	while (cin >> ans)
	{
		ans.stringlow();
		for (int i = 0; i < 3; i++)
		{
			if(ans == rgb[i])
			{
				cout << "That's right!\n";
				success = true;
				break;
			}
		}
		if(success)
			break;
		else
			cout << "Try again\n";
	}
	return 0;
}

[Peter87]

When str is returned from operator+ it is copied using the copy constructor. In some situations the copy can be optimized away but don't assume it is.

String s2 = "Please enter your name: ";
This is actually equivalent to
String s2(String("Please enter your name: "));
All compilers that I know of will optimize away the call to the copy constructor in this situation so you will most likely not experience any problems, but to be safe it's best to always follow the rule of three that I linked earlier.

If you disable the copy constructor (by making it private or using the C++11's =delete syntax) you will get error messages wherever the copy constructor is called. Do this if you don't want to have copy constructor, or just to see where the copy constructor calls are.

[ETAkoG1T]

I have defined all three :/ But the program is still not working. I think it is in the +operator function because when I did some experimenting the output changed but there was always a new problem. I think this is as close as i am going to get on my own. Any suggestions?

destructor:

String::~String()
{
	--num_strings;
	delete [] str;
}

copy constructor:

String::String( const String & st) 
{
	num_strings++;
	len = st.len;
	str = new char [len + 1];
	std::strcpy(str , st.str);
}

[Peter87]

Ah, yes you are right. There is a a problem in operator+ on line 8. You are adding len to the wrong pointer.

EDIT:
You are also never set len of the new object. If the length of the strings are stored in len you don't really need to use std::strlen in operator+.

[ETAkoG1T]

Thank you, I have fixed the problems you talked about. I can't really see exactly were the error is if it is not in the operator+ function. I edited my first post so that the whole program is included. I know its annoying having to look through the whole thing, but the help is appreciated if you have time.

[Peter87]

Not sure if you saw my edit. You still have to set str.len in operator+.

Other problems I found:
- Why is len set to 4 in the default constructor?
- You can change the const version of operator[] to have return type char. It isn't a problem but returning a copy instead of a reference is probably more efficient for such a simple type like char.
- The loops in stringlow(), stringup() and has(char) need to increment i.

[ETAkoG1T]

- You can change the const version of operator[] to have return type char. It isn't a problem but returning a copy instead of a reference is probably more efficient for such a simple type like char.

This is the code used in my c++ book, it may only be for practice purposes, idk.

- Why is len set to 4 in the default constructor?

Whoops, did some experimenting forgot to change it to one :P

- The loops in stringlow(), stringup() and has(char) need to increment i.

Thank you how could I forget, this solved my problem. I am not very surprised to find out that there was more than one problem :P hehe.

[Peter87]

Whoops, did some experimenting forgot to change it to one :P

It should be zero.