Solved

Explicitly deleting char pointer within object

Posted on 2003-11-05
21
655 Views
Last Modified: 2010-04-01
I have a char pointer within a object definition that I am allocating with 'new' in one of the object methods.  Should I (be able to) deallocate the memory via delete within the destructor?  The reading I have done suggests any time I allocate with 'new' I need to also deallocate with delete, but I seem to run into alot of incidences where my program chokes when I explicitly delete.   It chokes at the point of the delete cmd.

i.e.

class value
{
private:
      char      *val_label;            // Label for this value
public:
      
      value(char * stringPassed) :      val_label(NULL)
      {
            label(stringPassed);
      }

      ~value()      
      {
            delete val_label; // sometimes will bomb here, sometimes not?
      }


      char * label(const char *stringPassed)
      {
            if (val_label) delete val_label;
            val_label = new char[strlen(stringPassed)+1];
            strcpy(val_label, stringPassed);
            return val_label;
      }
}

void main ()
{
         value test = new value("mytestlabel");
}

Thanks,

Paul
0
Comment
Question by:str8dn
  • 8
  • 5
  • 4
  • +3
21 Comments
 
LVL 86

Expert Comment

by:jkr
ID: 9688865
If you are allocating the memory using

val_label = new char[strlen(stringPassed)+1];

you have to delete it like

delete [] val_label;
0
 
LVL 4

Expert Comment

by:n_fortynine
ID: 9688917
>>If you are allocating the memory using
>>val_label = new char[strlen(stringPassed)+1];
>>you have to delete it like
>>delete [] val_label;

I doubt that is the case though, jkr. (I might be wrong but) for most primitive variable types, delete and delete [] won't make a difference since you'll probably get all the memory allocated back. delete [] is only useful when deleting a bunch of non-primitive types, i.e. it will invoke all the destructors not just the [0]'s destructor.

I wonder if it'll be better if he wrote
if(val_label) delete val_label;
in his destructor.

Notice that in his constructor he also copied the pointers (not the content) and if somehow the other pointer got killed...
0
 
LVL 19

Expert Comment

by:Dexstar
ID: 9689139
n_fortynine:

> I doubt that is the case though, jkr. (I might be wrong but) for most primitive
> variable types, delete and delete [] won't make a difference

Nope.  JKR is 100% right about this.

> Notice that in his constructor he also copied the pointers (not the content)
> and if somehow the other pointer got killed...

What do you mean?  He allocates memory, and then calls strcpy.  It looks right to me.

> I wonder if it'll be better if he wrote
> if(val_label) delete val_label;
> in his destructor.

str8dn:
My suggestion is that you add a method called "clear()" (or whatever), that looks like this:

     void clear()
     {
          if ( val_label != NULL )
          {
               delete [] val_label;
               val_label = NULL;
          }
     }

And then invoke that method in your "label" function, and in your destructor.

Hope that helps,
Dex*
0
 
LVL 10

Expert Comment

by:Sys_Prog
ID: 9689148
As u are allocating memory for a char array using new, while deleting u should tell the compiler that u want delete the whole memory

In case u call just delete val_label, then there is no way for the compiler  to know whether your char pointer points to a single char/memory location OR char array/multiple memory locations.

Thus u have say delete [] val_label.

This works in the following manner

When u allocate memory for an array using new, the memory manager keeps track of the amount of memory allocated. Thus, just saying delete [] val_label is sufficient, U do not need to mention the no of locations to be deleted.

HTH
Amit
0
 
LVL 4

Expert Comment

by:n_fortynine
ID: 9689183
>>What do you mean?  He allocates memory, and then calls strcpy.  It looks right to me.
Yeah, I'm sorry, I overlooked that.
0
 
LVL 1

Author Comment

by:str8dn
ID: 9689314
Sorry for the distraction of my first mistake (delete not delete []) distraction
The real problem I am having is that even with
delete [] val_label

I am still bombing out at the point of the delete command.  Granted, my program is somewhat larger than what I have posted, so I may have created the problem elsewhere.
Does anyone know why the delete [] (char *) command would fail?  i.e something I should look for?

Thanks,

Paul
0
 
LVL 1

Author Comment

by:str8dn
ID: 9689344
BTW,

If the error is helpful it is:
>>>>>>>>>>
Debug Error!

Program ...

DAMAGE: after normal block (#608) at 0x00....
>>>>>>>>>>>>
I am not explicitly invoking any exceptions.

Thanks,

Paul
0
 
LVL 86

Expert Comment

by:jkr
ID: 9689417
>>DAMAGE: after normal block (#608) at 0x00....

Then, the above cannot be the code that causes that problem. Are you doing anything else?
0
 
LVL 12

Expert Comment

by:andrewjb
ID: 9689462
Your 'label' function returns the internal pointer as a char*
So, if you do something like

value *test = new value("mytestlabel");
char *fred = test->label("new thing");
delete [] fred;

That'd screw it, too

(And it should be value *test, not value test in your main(), but I presume that's an ooops, too)
0
 
LVL 1

Author Comment

by:str8dn
ID: 9689510
I am not actually using any of the return values for the methodsat this point.
0
Highfive Gives IT Their Time Back

Highfive is so simple that setting up every meeting room takes just minutes and every employee will be able to start or join a call from any room with ease. Never be called into a meeting just to get it started again. This is how video conferencing should work!

 
LVL 19

Expert Comment

by:Dexstar
ID: 9689520
str8dn:

Do either of these work without crashing?

     void main ()
     {
          value test("mytestlabel");
     }

or

     void main()
     {
          value* test = new value("mytestlabel");
          delete test;
     }

Dex*
0
 
LVL 1

Author Comment

by:str8dn
ID: 9689688
My progamr consists of the following:

Four classes, where each class holds several char * pointers (like val_label in orig post).
Classes also have "next" methods that are used to facilitate the linked list looping.
All pointers a initally set to NULL ( I thought this would help protect against delete probs)

Class value includes:
   value *next  // pointer to next value in linked list of values (assoc w/ tag)
   char  *label  // and other char *'s handled the same way as in orig post
   
Class tag includes:
    tag   *next  // pointer to next tag in linked list of tags (assoc w/ category)
    char *label // and other char *'s handled the same way as in orig post
    value *first_value  // first value in linked list of values assoc with this tag

Class category includes:
    category *next  // pointer to next category in linked list of categories (assoc w/ input_file)
    char *label // // and other char *'s handled the same way as in orig post
    tag first_tag  // first tag in linked list of tags assoc with this tag

Class input_file  includes:
    char *label // and other char *'s handled the same way
   value *first_category  // first value in linked list of value assoc with this tag
   read () // method that creates all of the category, tag, and value objects accd to a fiile.

All objects are created via new and explicitly deleted (by iterating through the linked
objects in the parent object's destructor)

I am able to display the conents of the file correctly from memory, but one of the the delete [] (char *) commands fails (with above error) at first invocation, even though I can cout the pointer's address and value immediately before the call.

Thanks,

Paul
0
 
LVL 86

Expert Comment

by:jkr
ID: 9689707
Hum, it's nice to see the data members, but - what about the code?
0
 
LVL 1

Author Comment

by:str8dn
ID: 9689732
Dex,

Both of those work.

In fact, The whole thing was working before I added another char *.  And it works (in appearance) until the point I try to delete it.

Thanks,

Paul
0
 
LVL 19

Expert Comment

by:Dexstar
ID: 9689797
They work?  Or they seem to work and then crash?  If they work, then what's the problem?

If they don't work, then you're probably doing something minor that is causing you problems, but we won't be able to find it without seeing all of your code.

Dex*
0
 
LVL 1

Author Comment

by:str8dn
ID: 9689846
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#include <iostream.h>
#include <fstream.h>      // ifstream, ofstream objects

void del (char *ptr) // deletes the given pointer
{
      if (ptr != NULL) {
            delete [] ptr;
            ptr = NULL;
      }
}

char * strip (char *string) {
      // remove any leading or trailing spaces from the passed string
      // will also remove any trailing comments (beginning with #)

      unsigned i;
      
      // remove leading spaces
      for (i = 0;            
            i < strlen(string) && (string[i] == ' ' || string[i] == '      ');      
            i++);
      strcpy(string, &string[i]);

      // check string for comment and remove comment if found
      for (i = 0;      i <= strlen(string);i++) {
            if (string[i] == '#')  {// remove the trailing comment from here on
                  string[i] = '\0';
                  break;
            }
      }

      // remove trailing spaces
      for (i = strlen(string);
            i > 0 && (string[i] == '\n' || string[i] == 0
                         || string[i] == ' '  || string[i] == '      ');
            i--);

      string[i+1] = '\0';
      return(string);
}


class value
{
private:
      char      *val_label;            // Label for this value
      value      *val_next;            // Pointer to next tag in linked list
      char      *dispString;      // Display string for this value

public:
      
      value(char * stringPassed) :      val_label(NULL), val_next(NULL), dispString(NULL)
      {
            label(stringPassed);
            //cout << "CONSTRUCTOR inits value ("<<this<<") to " << label() << endl;
      }

      ~value()      
      {
            //cout <<"\nCalling DESTRUCTOR of value " << this << " with label "
                        //<< label() << endl;
            cout << "deleting val_label " << &val_label <<endl;
            if (val_label) del(val_label);
            cout << "deleting dispString " << &dispString <<endl;
            if (dispString) del(dispString);
      }


      char * label(const char *stringPassed)
      {
            if (val_label) del(val_label);
            val_label = new char[strlen(stringPassed)+1];
            strcpy(val_label, stringPassed);
            return val_label;
      }

      char *label()
      {
            return val_label;
      }

      value *next() // Returns pointer the next value if this is in a linked list
      {
            return val_next;
      }

      value *next(value *passedVal)  // Sets/Returns pointer the following
      {                                                       // value if this is in a linked list
            val_next = passedVal;
            return val_next;
      }

      char * displayString() // Returns string containing formatted display of the value
      {
            cout << "displaying value " <<this <<" = '"<< label() << "'"<<endl;
            if (dispString) {
                  cout << "value disp: trying to delete old dispString " << &dispString << endl;
                  del(dispString);
                  cout << "value disp: deleted old dispString" <<endl;
            }

            dispString = new char[strlen(val_label) + 1];
            cout << "value disp: new dispString is " << &dispString << endl;
            dispString[0] = '\0';
            strcpy(dispString, val_label);
            strcat(dispString, "\n");
            return dispString;
      }

};


class tag
{
private:
      char      *tag_label;            // Label for this tag
      tag            *tag_next;            // Pointer to next tag in linked list
      value      *firstVal;            // Pointer to first value in linked list of values for this tag
      char      *dispString;      // Display string for this tag

public:
      
      tag(char * stringPassed) :      tag_label(NULL), tag_next(NULL), firstVal(NULL), dispString(NULL)
      {
            label(stringPassed);
            //cout << "CONSTRUCTOR inits tag ("<<this<<") to " << label() << endl;
      }

      ~tag()      
      {
            //cout <<"\nCalling DESTRUCTOR of tag " << this << " with label "
                        //<< label() << endl;
            del(tag_label);

            // Destroy values for this tag
            value *thisVal = firstVal;
            while (thisVal) {
                  value *nextVal = thisVal->next();
                  delete thisVal;
                  thisVal = nextVal;
            }
      }


      char * label(const char *stringPassed)
      {
            if (tag_label) del(tag_label);
            tag_label = new char[strlen(stringPassed)+1];
            strcpy(tag_label, stringPassed);
            return tag_label;
      }

      char *label()
      {
            return tag_label;
      }

      tag *next() // Returns pointer the next tag if this is in a linked list
      {
            return tag_next;
      }

      tag *next(tag *passedTag)  // Sets/Returns pointer the following
      {                                                       // tag if this is in a linked list
            tag_next = passedTag;
            return tag_next;
      }

      value * addValue(char *stringPassed)
      {
                  //cout << "addValue: got string " << stringPassed << endl;
                  
                  value *newVal = new value(stringPassed);
                  
                  value *thisVal = firstVal;
                  while (thisVal) {
                        
                        if (! thisVal->next()) {
                                    thisVal->next(newVal);
                                    //cout << "set thisVal->next to " << newVal << " = "
                                           //<< newVal->label() <<endl;
                                    break;
                        }
                        thisVal = thisVal->next();
                  }

                  if (! firstVal) firstVal = newVal;
                        
                  return newVal;
      }

      char *displayString()
      {
            del(dispString);
            cout << "displaying tag " <<this <<" = '"<< label() << "'"<<endl;

            if (! firstVal) {
                  cout << "No values defined for tag " << tag_label <<endl;
                  return tag_label;
            }
            
            // Count to see how much memory the string will need.
            unsigned memLength = strlen(tag_label) + 4;  // Added three for " = "
                                                                          // and one for null end char
      
            value *thisVal = firstVal;
            while (thisVal)
            {
                  memLength += strlen(thisVal->displayString());
                  cout << "memLen now " << memLength << " from adding val '"
                         << thisVal->label() <<"'"<<endl;
                  thisVal = thisVal->next();      
            }
            cout << "Total memLen is " << memLength << endl;

            // create new display string
            if (dispString) del(dispString);

            dispString = new char[memLength];
            dispString[0] = '\0';
            
            // add the label
            strcpy(dispString, tag_label);
            strcat(dispString, " = ");
      
            //add the values
            thisVal = firstVal;
            while (thisVal != NULL)
            {
                  cout << "current dispString is " << dispString << endl;
            
                  strcat(dispString,thisVal->displayString());
                  cout << "Next val is " << thisVal->next() <<endl;
                  thisVal = thisVal->next();
                  cout << "thisVal set to " << thisVal <<endl;
            }

            return dispString;
      }


};


class category
{
private:
      char            *cat_label;            // Label for this category
      category      *cat_next;            // Pointer to next category in file
      tag                  *firstTag;            // Pointer to linked list of tags for this category
      char            *dispString;      // Display string for this category

public:
      
      category(char * stringPassed) :      cat_label(NULL), cat_next(NULL), firstTag(NULL),
                                                      dispString(NULL)
      {
            label(stringPassed);
            cout << "CONSTRUCTOR inits category ("<<this<<") to " << label() << endl;
      }

      ~category()      
      {
            cout <<"\nCalling DESTRUCTOR of category " << this << " with label "
                        << label() << endl;
            del(cat_label);
            del(dispString);

            // Destroy tags for this category
            tag *thisTag = firstTag;
            while (thisTag) {
                  cout << "Destroying tag: " << thisTag->label() <<endl;
                  tag *nextTag = thisTag->next();
                  delete thisTag;
                  thisTag = nextTag;
            }

      }


      char * label(const char *stringPassed)
      {
            if (cat_label) del(cat_label);
            cat_label = new char[strlen(stringPassed)+1];
            strcpy(cat_label, stringPassed);
            return cat_label;
      }

      char *label()
      {
            return cat_label;
      }

      category *next() // Returns pointer the next category if this is in a linked list
      {
            return cat_next;
      }

      category *next(category *passedCat)  // Sets and returns pointer the following
      {                                                       // category if this is in a linked list
            cat_next = passedCat;
            return cat_next;
      }

      tag * addTag(char *stringPassed)
      {
                  //cout << "addTag: got string " << stringPassed << endl;
                  
                  tag *newTag = new tag(stringPassed);
                  //cout << "addTag: created newTag " << newTag << " = "
                         //<< newTag->label() << endl;
                  
                  tag *thisTag = firstTag;
                  while (thisTag) {
                        if (! thisTag->next()) {
                                    thisTag->next(newTag);
                                    break;
                        }
                        thisTag = thisTag->next();
                  }

                  if (! firstTag)      firstTag = newTag;

                  return newTag;
      }

      
      char * displayString() // creates the display to use for a category
      {
            cout << "displaying category " <<this <<" = '" << label() <<"'"<< endl;
            if (dispString) del(dispString);

            if (! firstTag) {
                  cout << "No tags defined for " << label() <<endl;
                  return label();
            }

            // see how much memory dispString needs
      
            unsigned memLength = strlen(cat_label + 1); // the extra 1 is for the newline
            
            tag *thisTag = firstTag;
            while (thisTag)
            {
                  memLength += strlen(thisTag->displayString()) + 1;  // the extra 1 is for the newline
                  thisTag = thisTag->next();
            }


            // Create the display string
            //
            thisTag = firstTag;
            
            dispString = new char[memLength+3]; // one extra for each newline at end and one for null
            
            // add the name of this category
            strcpy(dispString, cat_label);  
            strcat(dispString, "\n");

            // add each of the tags
            while (thisTag)
            {
                  // display this tag and go on to the next one
                  strcat(dispString, thisTag->displayString());
                  thisTag = thisTag->next();
            }

            strcat(dispString, "\n\n");  // add two blank lines at end of each category
            return dispString;
      }

};

class xp_input
{

private:
      char            *filename;            // The xp_input file name  
      category      *firstCat;      // Pointer to first category in linked list of categories
                                          // for this file
      
      struct headerStruct
      {
            char **lines;
            unsigned nLines;
      } header;

public:
      xp_input (char * inputFileName) : filename(NULL), firstCat(NULL)
      {
            filename = new char[strlen(inputFileName)+1];
            strcpy(filename, inputFileName);
            cout << "CONSTRUCTOR inits xp_input ("<<this<<") file name to "
                        << filename << endl;
      }

      ~xp_input ()
      {
            cout << "\nDESTRUCTOR ending xp_input"<< endl;
            del(filename);
            
            // delete header info
            unsigned i = 0;
            for (i=0; i<header.nLines; i++) {
                  cout << "Destroying header line: " << header.lines[i] <<endl;
                  del(header.lines[i]);
            }

            // delete the categories pointed to by this file
            category *thisCat = firstCat;
            while (thisCat) {
                  cout << "Destroying category: " << thisCat->label() <<endl;
                  category *nextCat = thisCat->next();
                  delete thisCat;
                  thisCat = nextCat;
            }
      }

      category * addCategory(char *stringPassed)
      {
                  cout << "addCategory: got string " << stringPassed << endl;
                  
                  category *newCat = new category(stringPassed);
                  cout << "addCategory: created category (newCat)" << newCat << " = "
                         << newCat->label() << endl;
                  
                  category *thisCat = firstCat;
                  while (thisCat) {
                        if (! thisCat->next()) {
                                    thisCat->next(newCat);
                                    break;
                        }
                        thisCat = thisCat->next();
                  }

                  if (! firstCat) firstCat = newCat;
      
                  return newCat;
      }

      bool read();  // reads input file info into memory
      bool write(char *filename);  // reads input file info into memory

}
;

bool xp_input::read()
{
      const unsigned MAXLINE       = 256;      // max length expected for a single line in the file
      const unsigned MAXHEADER = 100;      // max number of header lines expected
      
      char *line = new char[MAXLINE];

      category * currentCat;      // Working category = latest encountered during read
      tag             * currentTag;  // Working tag

      ifstream inFile(filename); // open the file for readin

      
      // HEADER
      // Get whole header - header lines are denoted by a '#'
      // as the first character.
      // The header ends at the first non-blank line that
      // does not start with '#'
      
      unsigned hCount = 0;
      
      inFile.getline(line, MAXLINE);
      //cout << "First line is " << line << endl;

      char **hLines = new char *[MAXHEADER];
      while (inFile && (line[0] == ' ' || line[0] == '#'))
      {
            hLines[hCount] = new char[strlen(line) + 1];
            //cout << "Allocated mem for hLines["<<hCount<<"]" <<endl;
            
            strcpy(hLines[hCount], line);
            //cout << "Set hLines["<<hCount<<"] to " << hLines[hCount] <<endl;
            
            inFile.getline(line, MAXLINE);
            hCount++;
            
      }

      header.nLines = hCount;
      header.lines  = hLines;

      // DATA

      while (inFile)
      {
            inFile.getline(line, MAXLINE);
            
            // CATEGORY
            if (line[0] == '[') {
                  currentCat = addCategory(line);
                  continue;
            }

            // Find out if this line is a tag line by checking for equal sign.
            bool line_is_tag = false;

            // TAG
            unsigned eqIndex;
            for (eqIndex = 0; eqIndex<strlen(line); eqIndex++)
                  if (line[eqIndex] == '=') { line_is_tag = true; break;}

            if (line_is_tag) {
                  // isolate label portion of tag line
                  line[eqIndex] = '\0';
                  char * tag_portion   = line;
                  
                  // remove extraneous spaces
                  strip(tag_portion);

                  // VALUE type 1 (same line as tag)
                  char * value_portion = &line[eqIndex+1];
                  strip(value_portion);


                  currentTag = currentCat->addTag(tag_portion);
                  currentTag->addValue(value_portion);
            }
            else {
                  // VALUE type 1 (already isolated value as line)
                  strip(line);
                  if (! strlen(line)) continue;
                  currentTag->addValue(line);
            }

            //cout << "read: bottom of loop" << endl;
            
      }
      inFile.close();

      return true;
}

bool xp_input::write(char * file_name)
{

      ofstream outFile(file_name); // open the file for reading
      
      // print the header
      if (header.nLines) {
            unsigned i;
            for (i=0 ; i< header.nLines; i++) {
                  outFile << header.lines[i] <<endl;
            }
      }
      else {
            cout << "No header info set." <<endl;
      }

      // print the data
      category *current = firstCat;
      if (! current) {
            cout << "firstCat was never set " << endl;
            exit(1);
      }

      while (current != NULL)
      {
            cout << "writing cat " << current->label() << endl;
            outFile << current->displayString();
            current = current->next();
      }
      outFile.close();

      return true;
}



bool main (const int argc, const char *argv[])
{

      xp_input myfile("../testdome.xp_input");
      cout << "Reading inputfile "<< endl;
      bool successfulRead = myfile.read();
      if (! successfulRead) {
            cout << "read failed." << endl;
            return false;
      }
      
      myfile.write("testout.txt");
      cout << "Finished read" << endl;
      
      return true;
}


Given that I am just picking up C++, there are probably quite a few no no's here.  All advice is welcome.  A link to a straight forward "how to debug" tutorial would also be well received.

Thanks,

Paul
            
0
 
LVL 86

Accepted Solution

by:
jkr earned 500 total points
ID: 9689883
Something that *will* cause some trouble is

         dispString = new char[strlen(val_label) + 1];
         cout << "value disp: new dispString is " << &dispString << endl;
         dispString[0] = '\0';
         strcpy(dispString, val_label);
         strcat(dispString, "\n");
         return dispString;

By using 'strcat()', you are most likely to overwrite some memory after the end of the allocated area of

         dispString = new char[strlen(val_label) + 1];

as you are adding '\n' *and* a terminating zero. This would explain the error message. Either use

         dispString = new char[strlen(val_label) + 2];

or (what I actually would suggest) use STL strings to avoid that sort of trouble.
0
 
LVL 1

Author Comment

by:str8dn
ID: 9689886
Sorry for the long post, BUt it seemed apparent I had been quite unsuccessful in parsing things out.

It bombs out at the delete command after "value disp: trying to delete old dispString"
 
Thanks,

Paul
0
 
LVL 1

Author Comment

by:str8dn
ID: 9690031
Thanks jkr,

How do you find errors so quickly?  Are you just looking through each of the programs, or running them in some sort of debug?

Thanks again, that simple +1 instead of 2 seemed to be the source.

Paul
0
 
LVL 19

Expert Comment

by:Dexstar
ID: 9690045
Yeah, that is the sort of thing we wouldn't have seen without you posting your whole code.  Good find, jkr!

str8dn : Listen to jkr's other suggestion and use std::string ... It makes all these issues go away.

D*
0
 
LVL 86

Expert Comment

by:jkr
ID: 9690157
>>Are you just looking through each of the programs

In this case, looking at the code (with some experience :o) helped...
0

Featured Post

How your wiki can always stay up-to-date

Quip doubles as a “living” wiki and a project management tool that evolves with your organization. As you finish projects in Quip, the work remains, easily accessible to all team members, new and old.
- Increase transparency
- Onboard new hires faster
- Access from mobile/offline

Join & Write a Comment

Container Orchestration platforms empower organizations to scale their apps at an exceptional rate. This is the reason numerous innovation-driven companies are moving apps to an appropriated datacenter wide platform that empowers them to scale at a …
Basic understanding on "OO- Object Orientation" is needed for designing a logical solution to solve a problem. Basic OOAD is a prerequisite for a coder to ensure that they follow the basic design of OO. This would help developers to understand the b…
The viewer will learn how to use the return statement in functions in C++. The video will also teach the user how to pass data to a function and have the function return data back for further processing.
The viewer will learn additional member functions of the vector class. Specifically, the capacity and swap member functions will be introduced.

743 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

11 Experts available now in Live!

Get 1:1 Help Now