[C++] Destruktor stürzt ab

[MastaZulu]

Hi,
bin grad beim Lernen für meine Programmierungs-Prüfung und übe zur Zeit die Implementierung einer String-Klasse.

Soweit so gut es läuft auch alles, bis auf den Destruktor. Und zwar habe ich eine Funktion namens "repeat" geschrieben, die einen String mehrmals hintereinander schreibt (was sie auch tut). Wird jedoch der Destruktor für dieses String-Objekt aufgerufen, schmiert das Programm ab.

Hier mal ein bisschen Code:

...

// Destruktor
CString::~CString()
{
	cout << "Destroying \"" << this->str << "\"" << endl;
	delete [] this->str;
}

...

// Repeat-Funktion
CString& CString::repeat(unsigned int rep)
{
	cout << "\nRepeating \"" << this->str << "\" " << rep << " times.";

	char* temp = new char[(this->len*rep)+1];
	temp = strcpy(temp, this->str);
	if(rep >= 1)
	{
		for(unsigned i=0; i<=(rep-1); i++)
			temp = strcat(temp, this->str);
		delete [] this->str;
	   this->str = temp;
   }

	cout << " Repeated String: \"" << this->str << "\"" << endl << endl;

	return *this;
}

...

Hier der Aufruf der Funktion:

#include "strings.h"

void main(void)
{
...
	CString S("Hi!_2");
	S.repeat(2);
...
}

Also sobald in der Repeat-Funktion etwas verändert wird, schmiert das Programm ab. Hab in anderen Funktionen (z.B. der +-Operator-Überladung) ähnliche Codezeilen was das Verändern des aufrufenden Objektes angeht, die gehen aber alle durch.

Wo liegt mein Fehler?

Gruß
MastaZulu

 

[Kamikatze]

Was machst du da?!

temp = strcpy(temp, this->str);

Du kopierst den String in temp und überschreibst ihn dann mit dem Rückgabewert von strcpy(...)?

EDIT: Okay... liefert ohnehin temp zurück... ist aber unnötig.

Aber hab vermutlich einen anderen Fehler entdeckt.
Du überschreibst deinen zur Verfügung stehenden Speicher.

Du reservierst genau so viel Speicher, wie in rep steht, d.h. wenn rep == 1 hast du die Länge für 1 String zur Verfügung ("Test"). Sollte einmal wiederholen aber nicht schon 2 Strings bedeuten ("TestTest")?

Also, du musst auch den Speicher für den normalen String und nicht nur für die Wiederholungen berücksichtigen:

char* temp = new char[(this->len*(1+rep))+1];

Das Ganze hat dann aber nicht wirklich etwas mit dem Destruktor zu tun.

 

[MastaZulu]

öhm, thx, das wars

wobei ich aber auch nicht verstehe, was das mit dem destruktor zu tun hat... das entzieht sich meiner erkenntnis. nunja, vielen dank! @ kamikatze

gruß

MastaZulu

 

[joe67]

Auch wenn's vielleicht schon etwas zu spät ist, das Problem aber möglicherweise trotzdem noch nicht gelöst ist, will ich auch ein paar Anmerkungen machen.

Zunächst einmal wäre es noch wichtig deinen Constructor zu kennen. Ohne Informationen darüber, wie der Speicher von str reserviert wird, ist es schwierig rauszufinden, wieso der Code des Destructors nicht läuft.

Desweiteren wäre die Stelle im Code interessant, an der len einen Wert zugewiesen bekommt. Denn davon hängt ja ab, wie groß der Speicherbereich ist, der für temp reserviert wird. Sollte nämlich temp zu wenig Speicher bekommen, dann kriegst du mit strcpy - spätestens aber mit strcat - ein Problem. Wenn mit strcat nämlich über die Grenzen des reservierten Speichers für temp hinausgeschrieben wird, dann kann alles mögliche passieren.

Ansonsten emfpehle ich dir str nicht zu verändern und vor allem nicht an mehreren Stellen mittels delete den reservierten Speicher freizugeben. Sowas wird bei größeren Programmen schnell zur "tödlichen" Falle.

Ich würde die Methode repeat anders schreiben (entscheidende Änderungen habe ich rot markiert):

CString& CString::repeat(unsigned int rep)
{
	cout << "\nRepeating \"" << this->str << "\" " << rep << " times.";

	char* temp = new char[([COLOR="Red"]strlen(this->str)[/COLOR]*rep)+1];
[COLOR="Red"]	if( rep >0 ) {[/COLOR]
                temp = strcpy(temp, this->str);

		for(unsigned i=0; i<=(rep-1); i++)
			temp = strcat(temp, this->str);
[color="#ff0000"]//                delete [] this->str;              <- weg damit!
//                this->str = temp;                <- und auch damit
            	cout << " Repeated String: \"" << [COLOR="#ff0000"]temp[/COLOR] << "\"" << endl << endl;[/COLOR]
        }
[COLOR="Red"]        else 
        {
            	cout << " Repeated String: \"" << this->str<< "\"" << endl << endl;
        }
[/COLOR]
        delete [] temp;
	return *this;
}

Zum Vergleich will ich dir nochmal zeigen, wo dein Programm gefährlich ist:

CString& CString::repeat(unsigned int rep)
{
	cout << "\nRepeating \"" << this->str << "\" " << rep << " times.";

	char* temp = new char[(this->len*rep)+1];  [COLOR="Red"]// Was passiert, wenn rep = 0 ist? Was passiert wenn len einen falschen Wert besitzt?[/COLOR]
	temp = strcpy(temp, this->str);    [COLOR="Red"]// Was passiert wenn temp aufgrund einer Bedingung aus vorherigem Kommentar nur 1 char aufnehmen kann?[/COLOR]
	if(rep >= 1)
	{
		for(unsigned i=0; i<=(rep-1); i++)
			temp = strcat(temp, this->str);
		delete [] this->str;      [COLOR="#ff0000"]// was passiert wenn für str kein Speicher reserviert wurde?[/COLOR]
	   this->str = temp;    // An dieser Stelle geht der ursprüngliche Inhalt von str verloren, und kann für weitere Operationen nicht mehr verwendet werden.
   }

	cout << " Repeated String: \"" << this->str << "\"" << endl << endl;

	return *this;
}

Zu guter letzt noch eine Empfehlung für deinen Destructor: Prüfe vor dem delete, ob auch Speicher reserviert wurde.

z.B.

//Constructor
CMyClass::CMyClass()
{
    m_Pointer = NULL;
}

// Destructor
CMyClass::~CMyClass()
{
    FreePointer();
}


CMyClass::FreePointer()
{
     if( m_Pointer != NULL ) {
        delete[] m_Pointer;
        m_Pointer = NULL;
    }
}


CMyClass::SomeMethod()
{
    ...
    FreePointer();
    m_Pointer = new char[100];
    ...
}

j o e

 

[Raechaer]

Nur so nebenbei: Laut Standard ist delete 0 (und auch delete[] 0) absolut ok und macht einfach nichts, ist also nicht nötig erst noch auf != 0 zu überprüfen.