Explicitly deleting char pointer within object

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
LVL 1
str8dnAsked:
Who is Participating?

[Product update] Infrastructure Analysis Tool is now available with Business Accounts.Learn More

x
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

jkrCommented:
If you are allocating the memory using

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

you have to delete it like

delete [] val_label;
n_fortynineCommented:
>>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...
DexstarCommented:
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*
Bootstrap 4: Exploring New Features

Learn how to use and navigate the new features included in Bootstrap 4, the most popular HTML, CSS, and JavaScript framework for developing responsive, mobile-first websites.

Sys_ProgCommented:
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
n_fortynineCommented:
>>What do you mean?  He allocates memory, and then calls strcpy.  It looks right to me.
Yeah, I'm sorry, I overlooked that.
str8dnAuthor Commented:
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
str8dnAuthor Commented:
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
jkrCommented:
>>DAMAGE: after normal block (#608) at 0x00....

Then, the above cannot be the code that causes that problem. Are you doing anything else?
andrewjbCommented:
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)
str8dnAuthor Commented:
I am not actually using any of the return values for the methodsat this point.
DexstarCommented:
str8dn:

Do either of these work without crashing?

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

or

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

Dex*
str8dnAuthor Commented:
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
jkrCommented:
Hum, it's nice to see the data members, but - what about the code?
str8dnAuthor Commented:
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
DexstarCommented:
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*
str8dnAuthor Commented:
#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
            
jkrCommented:
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.

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
str8dnAuthor Commented:
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
str8dnAuthor Commented:
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
DexstarCommented:
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*
jkrCommented:
>>Are you just looking through each of the programs

In this case, looking at the code (with some experience :o) helped...
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
C++

From novice to tech pro — start learning today.