random crashes on delete []

Hi, im having trouble trying to write the String class, following along with "C++ without Fear". and most of the time my destructor for my string class crashes on delete [] ptr;

heres the sources.

main.cpp

Code:

---

#include "string.h"
//#include <iostream>
//#include <windows.h>

using namespace std;

int main(int argc, char** argv) {
    String str("hello world"); //crashes on this Strings destructor.
    String str2;
    String str3;
    String tmp;
    char *c = new char[500];
    delete c; //works fine.
    //cin.get();
    return 0;
}

---

string.h

Code:

---

#ifndef STRING_H
#define    STRING_H

//#include <cstring>
//#include <iostream>

class String {
private:
    char *ptr;
public:
    String();
    String(const char *s);
    String(const String &src);
    ~String();
    operator char*(){return ptr;}
    int operator==(const String &other);
    int operator>(const String &other);
    int operator<(const String &other);
    String& operator=(char *s);
    String& operator=(String &src);
private:
    void cpy(char *s);
};


#endif    /* STRING_H */

---

string.cpp

Code:

---

#include "string.h"

int strlen(const char *str){
    int i = 0;
    while(str[i] != '\0')
        i++;
    return i;
}

int strcmp(const char *c1, const char *c2){
    int i = 0;
    while(c1[i] == c2[i]){
        if(c1[i] == '\0')
            return 0;
        i++;
    }
    if(c1[i] > c2[i])
        return 1;
    if(c1[i] < c2[i])
        return -1;
}

void strcpy(char *d, const char *s){
    int i = 0;
    while(s[i] != '\0'){
        d[i] = s[i];
        i++;
    }
    d[i] = '\0';
}

String::~String(){
    delete []ptr;  //CRASHES HERE <-------------
}

String::String(){
    ptr = new char[1];
    ptr[0] = '\0';
}

String::String(const char *s){
    int n = strlen(s);
    ptr = new char(n+1);
    strcpy(ptr, s);
}

String::String(const String& src){
    int n = strlen(src.ptr);
    ptr = new char(n+1);
    strcpy(ptr, src.ptr);
}

int String::operator ==(const String& other){
    return(strcmp(ptr, other.ptr) == 0);
}

int String::operator <(const String& other){
    if(strcmp(ptr, other.ptr) == -1)
        return 1;
    else
        return 0;
}

int String::operator >(const String& other){
    if(strcmp(ptr, other.ptr) == 1)
        return 1;
    else
        return 0;
}

void String::cpy(char* s){
    delete []ptr;
    int n = strlen(s);
    ptr = new char[n + 1];
    strcpy(ptr, s);
}

String& String::operator =(char* s){
    cpy(s);
    return *this;
}

String& String::operator=(String &src){
    cpy(src.ptr);
    return *this;
}

---

heres the call stack when it crashes

Code:

---

ntdll!TpWaitForAlpcCompletion ()
?? ()
ntdll!RtlLargeIntegerDivide ()
?? ()
ntdll!RtlCopyExtendedContext ()
?? ()

---

i know it shouldnt be my strcpy, strcmp, strlen functions because i tried it using the standard functions and no change.

another thing to note is that it only crashes when the String::String(const char *s); constructor is used.
thanks for any help. :confused:

> ptr = new char(n+1);
Creates a single char, and initialises it with n+1

ptr = new char[n+1];
Creates an array of n+1 chars

wow how did i miss that. id spent hours trying to figure out was was wrong, lol. it works now. thanks for the help.

Quote:

---

Originally Posted by Salem

> ptr = new char(n+1);
Creates a single char, and initialises it with n+1

ptr = new char[n+1];
Creates an array of n+1 chars

---

Also, what's the point of dynamically allocating a character array of size 1 just so that you can put a terminating null char in it to indicate that the string is empty? Why not just set ptr to null and be done with it? You would have to adjust the logic of some of your methods, but I think it would be worth it.

Your strlen returns length + 1, which is wrong.
Your strcmp returns wrong if str1 is shorter than str2.
Your strcpy is a horrible, horrible function. You have your class, so why do you have strcpy? Remove it!

Quote:

---

Originally Posted by Elysia

Your strcpy is a horrible, horrible function. You have your class, so why do you have strcpy? Remove it!

---

I think he's only using it as an implementation detail to copy string data between instances. Notice that there is no mention of strcpy in his header file.

In such case, I'd recommend breaking out the whole copy procedure into its own function and make it a private member.
strcpy by itself is very, very dangerous. If insisting on making a private strcpy, at least provide buffer sizes to make sure you don't cause buffer overflows.

There are various const-correctness issues with his code as well. Plus, operator ==, < and > should return a bool, not an int. I'll post a modified version of the original header file to show what the signature of the various methods should be:

Code:

---

#ifndef STRING_H
#define    STRING_H

//#include <cstring>
//#include <iostream>

class String
{
private:
    char *ptr;
   
public:
    String();
    String(const char *s);
    String(const String &src);
    ~String();
    operator char*(){return ptr;} // <-- If I were you, I'd get rid of this one.
                                  // Giving the outside world non-const access
                                  // to the string's internal data is a
                                  // a horrible idea.

    operator const char*() const {return ptr;}
    bool operator==(const String &other) const;
    bool operator>(const String &other) const;
    bool operator<(const String &other) const;
    String& operator=(const char *s);
    String& operator=(const String &src);
private:
    void cpy(const char *s);
};

#endif    /* STRING_H */

---

antred makes a good point concerning your String class' conversion function to char*, but also, consider either declaring the conversion function to const char* to be explicit (a feature from C++11), or use a named conversion function instead (like std::string's c_str member function). Since you already allow implicit conversions from const char* to String, allowing implicit conversions from String to const char* can make things confusing.

Another thing: your class definition does not need anything from <cstring> or <iostream>, so you should move those header inclusions into the source file instead (though I notice that you commented them out). Actually, you should not need <iostream> at all. You might want to overloaded operator>> and operator<< for input/output streams, in which case you should #include <iosfwd> in the header then #include <istream> and #include <ostream> in the source file.