Memory Leak Question

[wishknew]

Hello Friends,

First of all, I am using g++ compiler under linux.
I have a question whether my code prone to memory leak.

Here is the prototype:

#ifndef __DELIMITEDSTRING_H__
#define __DELIMITEDSTRING_H__
#include <iostream>
#include <vector>

/* ****************************************
 *  Class  : DelimitedString 
 *  Desc   : This class is intended to
 *           perform string split, with delimiter ','
 *           hrmh,wisnu,widiarta --> getElement(0) returns hrmh
                                 --> getElement(2) returns widiarta
 *****************************************/

using namespace std;

class DelimitedString {
   public:
     DelimitedString();
     DelimitedString(string delimited);
     DelimitedString(string delimited, string delimiter);
     ~DelimitedString();
     
     string getText();
     void setDelimitedString(string input);
     void setDelimiter(string delimiter);
          
     int getCount();
     string getElement(int index);
   private:
     string input;
          
     int count;
     string delimiter;
     vector<string> elements; 
     string trim(const string inputString);
     
     void parse();
};

//The method parse and destructor are below

void DelimitedString::parse() { 
   string* token = new string("");     
   elements.clear();   
   input = trim(input);

   int index;
   string newInput(input + delimiter);
   index = (int)newInput.find(delimiter); 
   
   for(;index != -1; index = (int)newInput.find(delimiter)) {
        try { 
          token = new string(trim(newInput.substr(0, index)));                    
          elements.push_back(*token);            
        }
        catch (const exception& error) {
          cerr << "Error in DelimitedString::Parse : " << error.what() << endl;
        }
       
        try {
           newInput = newInput.substr(index + 1) + "";           
        }
        catch (const exception& error) {
          cerr << "Error in DelimitedString::Parse : " << error.what() << endl;
        }           
   }
}

DelimitedString::~DelimitedString() {
   elements.~vector<string>();
}

My questions are :

1. Can the statement string* token = new string(""); cause memory leak? Can I just use string? (I have tried but since it is local, it caused segmentaton fault)
2. If yes, how to avoid that?
3. I have tried delete token at the bottom, but since it still being used by vector<string>, it caused segmentation fault.
4. Should I use vector<string*> instead of vector<string>? Before I call the statement in the current destructor, I iterate the vector and delete the element one by one. Is it better or even worse?
5. What does the statement elements.~vector<string>(); mean?

Thank you in advance.

Wisnu Widiarta

[Valmont]

This is faulty code to begin with. And yes, it does cause memory leaks.

[wishknew]

How to avoid that, Valmond? Please answer all the questions. Thanx

WW

[Valmont]

There is a lot to be done to give the proper example. Post the complete code. Or upload it so I can download it. Or mail the source and headers zipped to: my email
Then I will post it neatly for you.

[Valmont]

The original code is this:

#include <iostream>
#include <cstring>
#include "DelimitedString.h"
     
DelimitedString::DelimitedString() {
   this->delimiter = ",";
   this->count = 0;
}

DelimitedString::DelimitedString(string delimited) {
   this->input = delimited;
}

DelimitedString::DelimitedString(string delimited, string delimiter) {
   this->input =  delimited;
   this->delimiter = delimiter;
      
   parse();
}

DelimitedString::~DelimitedString() {
   elements.~vector<string>();
}

string DelimitedString::getText() {
   return input;
}

void DelimitedString::setDelimiter(string delimiter) {
   this->delimiter = delimiter;
   parse();
}

void DelimitedString::setDelimitedString(string input) {
   this->input = input;
   parse();
}

int DelimitedString::getCount() {
   count = elements.size();
   return count;
}

string DelimitedString::getElement(int index) {
   return elements.at(index);
}

void DelimitedString::parse() { 
   string* token = new string("");     
   elements.clear();   
   input = trim(input);

   int index;
   string newInput(input + delimiter);
   index = (int)newInput.find(delimiter); 
   
   for(;index != -1; index = (int)newInput.find(delimiter)) {
        try { 
          token = new string(trim(newInput.substr(0, index)));                    
          elements.push_back(*token);            
          //elements.push_back(trim(newInput.substr(0, index))); not working
        }
        catch (const exception& error) {
          cerr << "Error in DelimitedString::Parse : " << error.what() << endl;
        }
       
        try {
           newInput = newInput.substr(index + 1) + "";           
        }
        catch (const exception& error) {
          cerr << "Error in DelimitedString::Parse : " << error.what() << endl;
        }           
   }
}

string DelimitedString::trim(const string inputString) {
   string result("");   
   if (inputString != "") {
      int index;
      result = inputString;
      index = (int) result.find_last_not_of(' ');
      if (index != -1)
        result = result.substr(0, index + 1);
      index = (int) result.find_first_not_of(' ');
      if (index != -1)
        result = result.substr(index);         
   }
   return result;
}

You don't need a pointer to std::string.
So change it from
[ocde]
string* token = new string("");
[/code]
to

string token("");

Then you also have to change this:

token = new string(trim(newInput.substr(0, index)));                    
elements.push_back(*token);

to:

token =  trim(newInput.substr(0, index));
elements.push_back(token);

But basically you could optimize the whole thing:

The header

#ifndef __DELIMITEDSTRING_H__
#define __DELIMITEDSTRING_H__

#include <vector>
#include <string>

using namespace std;

class DelimitedString
{
   public:
     DelimitedString();
     DelimitedString(string delimited);
     DelimitedString(string delimited, string delimiter);
     ~DelimitedString();
     
     void parse();
     
     string getText() const;
     void setDelimitedString(string input);
     void setDelimiter(string delimiter);
          
     int getCount() const;
     string getElement(int index) const;
   private:
     string sInput_;
     int nCount_;
     string sDelimiter_;
     vector<string> Elements_;
     void trim(string& inputString);
};

#endif

#include "DelimitedString.h"
     
DelimitedString::DelimitedString() : sDelimiter_(","), nCount_(0)
{ }

DelimitedString::DelimitedString(string delimited) : sInput_(delimited)
{ }

DelimitedString::DelimitedString(string delimited, string delimiter) :
  sInput_(delimited), sDelimiter_(delimiter)
{ }

DelimitedString::~DelimitedString()
{ }

//--------------------------------------------------------

string DelimitedString::getText() const
{
  return sInput_;
}

void DelimitedString::setDelimiter(string delimiter)
{
  sDelimiter_ = delimiter;
}

void DelimitedString::setDelimitedString(string input)
{
  sInput_ = input;
}

int DelimitedString::getCount() const
{
  return Elements_.size();
}

string DelimitedString::getElement(int index) const
{
  if(Elements_.size() > 0)
  {
    return Elements_.at(index);
  }
  else
  {
    throw "Could not split";
  }
}

void DelimitedString::parse()
{
  //Remove spaces first.
  trim(sInput_);
  string::size_type first;
  for(string::size_type index=0 ; index < sInput_.size(); )
  {
    first = sInput_.find_first_not_of(sDelimiter_, index);
    index = sInput_.find_first_of(sDelimiter_, first);
    if(first != string::npos)
    {
      Elements_.push_back(sInput_.substr(first, index-first));
    }
  }
}


void DelimitedString::trim( string& inputString)
{
  
  for(unsigned i = 0; i < inputString.size(); ++i)
  {
    if(inputString[i] == ' ')
    {
      inputString.erase(i, 1);
    }
  }
}

But now in main, you have to call parse explicitly:

#include <iostream>
#include "DelimitedString.h"

using namespace std;

int main(int argc, char *argv[])
{
  DelimitedString ds;
  ds.setDelimitedString(" hrmh ,wisnu,widiarta ");
  ds.setDelimiter(",");

  ds.parse();

  cout<<ds.getCount()<<endl;
  cout<<ds.getElement(0)<<endl;
  cout<<ds.getElement(1)<<endl;
  cout<<ds.getElement(2)<<endl;
  
  system("PAUSE");  
  return 0;
}

[wishknew]

Ok, Valmont thanx!

I have changed the string* into string, so I can avoid the new operator. But I didn't see the use of private variable nCount_ in your recommendation code. I prefer use the size of vector size instead of maintaining the vector count. And I think your trim function will erase all of spaces in a string which not something I want, since my trim is intended to leftTrim and rightTrim at the same time.

Thanks again for inline functions in header file and the use of reference in trim.

Best regards,

Wisnu Widiarta