Re: MyString Class

From: Karl Heinz Buchegger (kbuchegg_at_gascad.at)
Date: 10/24/03


Date: Fri, 24 Oct 2003 11:26:01 +0200


Karl Heinz Buchegger wrote:

Sorry. Hit Send by accident.

>
> Luis wrote:
> >
> > Below is my project. My Code is done, but when I try to compile it, my IDE
> > crahses (again). I fixed up all the things you guys said I should, except
> > for some which would not meet the project requiremnts. here is my code. it
> > compiles, but crashes winsioux.
>
> Learn to use a debugger.
> I compiled your program and run it under the debugger.
> The first crash happens in ...
>
> > istream& operator>>(istream& in,MyString &inString)
> > {
> >
> > char temp[128];
> > delete [] inString.string;
>
> ^
> ... at this line |
>
> which is a string indication that something with the way you allocate
> memory is flawed. So lets look at the constructor first:
>
> > MyString::MyString()
> > {
> >
> > string = new char[0];
> > string[0] = 0;
> >
> > }
>
> Oha. You allocate allocate an array of size 0 (which is legal)
> but yet assign a value to an imaginary first array element.
> If an array has size 0, then there is no element 0. If an
> array has as its only element the element 0, then it has
> size 1 (as there is one element in the array).
> Remember: The biggest valid index into an array is always
> 1 less then the size of the array (since counting starts at 0)
>
> string = new char[ 1 ];
> string[0] = 0;
>
> OK. First bug fixed. Recompile, starting the debugger.
>
> The next crash happens, when in the loop:
>
> > while (!eof(in)) {
> > MyString s; // creates an empty string
> > if (in.peek() == '#') { // peek at char, comments start with #
> > in.ignore(128, '\n'); // skip this line, it's a comment
> > } else {
> > in >> s;
> > cout << "Read string = " << s << endl;
> > }
> > }
>
> the variable s goes out of scope. Again it has something to do with
> memory management, since th crash happens deep in systems code, which
> originated at delete. So lets look at the history of s:
> It has been created, then operator>> assigns a new value to it and
> then it gets deleted. Fine. The ctor is correct, so the problem must
> be in operator>>. Lets take a closer look:
>

istream& operator>>(istream& in,MyString &inString)
{

    char temp[128];
    delete [] inString.string;
    in>>temp;
    strcpy(inString.string,temp);

    return in;
}

Oha.
I can see, that you delete inString.string
But i can't see where new memory is allcoated. Thus
the strcpy copies the characters to memory which
no longer belongs to the program. Worse then that
when the time has come for the dtor to delete the memory,
it will delete the same memory twice.
Lets fix that:

    char temp[128];
    delete [] inString.string;
    in>>temp;
    inString.string = new char [ strlen( temp ) + 1 ];
    strcpy(inString.string,temp);

    return in;
}

Besides: your usage of eof in the reading loop is wrong. See the
FAQ to figure out why.

OK. Fixing the bug, recompile and starting the debugger takes just
a couple of seconds.

The next bug happens in the next loop:

    cout << endl << "----- now, line by line" << endl;
    ifstream in2("string.data");
    assert(in2);
    while (!eof(in2)) {
        MyString s; // creates an empty string
        if (in2.peek() == '#') { // peek at char, comments start with #
            in2.ignore(128, '\n'); // skip this line, it's a comment
        } else {
            s.read(in2, '\n');
            cout << "Read string = " << s << endl;
        }
    }
    in2.close();

Same symptoms: The crash happens when variable s goes out of scope. With the
experience from the previous bug, I immediatly acuse function MyString::read

void MyString::read(istream &inString,char delimit)
{
    char temp[128];
    delete []string;
    inString.getline(temp,127,delimit);
    strcpy(string,temp);
 }

Same problem: you delete but don't allocate.

    inString.getline(temp,127,delimit);
    string = new char [ strlen( temp ) + 1 ];
    strcpy(string,temp);

Fixing the bug and starting the debugger again takes a couple of seconds.
And on we go, the next crash happens in:

 MyString operator+(const MyString left,const MyString right)
 {
      MyString temp;
     //temporary mystring to hold contents of left.string+right.string
     temp.string = new char[strlen(left.string)+strlen(right.string)+1];
     //dynamic array size of left.string+right.string+1
     strcat(left.string,right.string);
     //make left.string+right.string into left.string
     strcpy(temp.string,left.string);

Hmm. HOw can you strcat something to left.string.
left.string has still its old size, and since your array sizes
are always tailored to what is needed, this is definitly to short
to store the result of the strcat.

I guess you ment:

     strcpy( tmp.string, left.string );
     strcat( tmp.string, right.string );

Fixing that bug and starting the debugger again takes a couple of seconds.
And on we go, the next crash happens ...

No more crashes.

All i nall I have to say you made a typical newbie mistake.
You wrote too much code without intermediate testing. Write
*one* function, write *one* operator and test it! Only after
that new function works as expected add the next one.

-- 
Karl Heinz Buchegger
kbuchegg@gascad.at