Like Tree1Likes
  • 1 Post By hk_mp5kpdw

Which of these is a better way to destruct a vector?

This is a discussion on Which of these is a better way to destruct a vector? within the C++ Programming forums, part of the General Programming Boards category; So I was working on a destructor for a vector of object pointers and I wrote up a little test ...

  1. #1
    Registered User
    Join Date
    Dec 2009
    Posts
    41

    Which of these is a better way to destruct a vector?

    So I was working on a destructor for a vector of object pointers and I wrote up a little test program to be able to get rid of any memory leaks. I have 2 methods. the first is to loop through the vector's elements and delete them, and the second uses an iterator. both seem to free the memory and are pretty tough to break. Is there a benifite to using one method over the other? and why?

    note, this is a very alive test program, there's a few lines that don't do anything and some leftovers from older tests. the functions I'm most interested are baddelete() and IterDelete();


    Code:
    /*
     * ClassRoom.h
     *
     *  Created on: Jan 20, 2012
     *      Author: 
     */
    #include <string.h>
    #include <vector>
    #include <fstream>
    using namespace std;
    
    #ifndef CLASSROOM_H_
    #define CLASSROOM_H_
    
    
    
    
    class ClassRoom{
    public:
    	ClassRoom();
    	~ClassRoom();
    	void Pu........(string*,int,int);
    	void printIt();
        void BadDelete();
        void IterDelete();
        void otherVec();
        void PushFile(char *, char *);
    private:
        
      struct FileEntry{
        char * fileName;
        char * path;
       };
        
    	struct Person{
    		string * name;
    		int * age;
    
    	};
    	vector<Person*> * theRoom;
        vector<FileEntry * > * itsFileSeqPtr;
    	vector<Person*> * aClass;
    	vector<Person*> * anotherRoom;
    	vector<Person*> * roomThree;
        ofstream * file;
    };
    
    
    #endif /* CLASSROOM_H_ */
    Code:
    /*
     * VectorTest.cpp
     *
     *  Created on: Jan 20, 2012
     *      Author: 
     */
    
    
    
    
    #include <iostream>
    
    #include <string.h>
    #include <vector>
    #include "ClassRoom.h"
    using namespace std;
    
    
    
    	ClassRoom::ClassRoom():
    		theRoom(new vector<Person*>(15)),
            itsFileSeqPtr( new vector<FileEntry *>()),
            file(NULL){
                
    		//aClass(new vector<Person*>(6)),
    		//anotherRoom(new vector<Person*>(7)),
    		//roomThree(new vector<Person*>(9)){
    	}
    
    	ClassRoom::~ClassRoom(){
            BadDelete();
            IterDelete();
    	}
        void ClassRoom::BadDelete(){
            cout <<"deleting "<< theRoom->size() << " elems: the room" << endl;
            for(unsigned int i = 0; i<theRoom->size(); i++){
    			if(i < theRoom->size())
    				delete (*theRoom)[i];
    
    		}
    		//theRoom->clear();
    		delete theRoom;
    
    		cout << "destructed."<< endl;
            
        }
        
        void ClassRoom::IterDelete(){
            cout <<"deleting "<< itsFileSeqPtr->size() << " elems: the file" << endl;
            vector<FileEntry*>::iterator itFile;
    		for(itFile = itsFileSeqPtr->begin(); itFile != itsFileSeqPtr->end(); ++itFile){
                
                delete[] (*itFile)->path;
                delete[] (*itFile)->fileName;
                delete (*itFile);
            }
            delete itsFileSeqPtr;
        }
    	void ClassRoom::Pu........(string * name, int age, int elem){
    		Person * tempPerson = new Person;
            tempPerson->name=name;
            tempPerson->age= &age;
            (*theRoom).at(elem)= new Person;
    		(*theRoom).at(elem)->name = name;
    		(*theRoom).at(elem)->age = &age;
            delete tempPerson;
            }
        
        void ClassRoom::PushFile(char * name, char * path){
            FileEntry * fileEntryPtr = new FileEntry;
            char * swfFilePath("I\'m a string son");
            fileEntryPtr->fileName = new char[strlen(name) +1];
            fileEntryPtr->path = new char[strlen(path) +1];
            strcpy(fileEntryPtr->fileName,name);
            strcpy(fileEntryPtr->path,swfFilePath);
            itsFileSeqPtr->push_back(fileEntryPtr); 
            //delete fileEntryPtr;
            //delete swfFilePath;
            
    	}
        void ClassRoom::otherVec(){
            //itsFileSeqPtr
    
            
        }
    	void ClassRoom::printIt(){
    		vector<Person*>::iterator it;
    		 for(it = theRoom->begin(); it != theRoom->end(); ++it){
               if(*it !=NULL){
               cout <<"Personname::" << *((*it)->name) << endl;
                }
    		 }
             vector<FileEntry*>::iterator itFile;
    		 for(itFile = itsFileSeqPtr->begin(); itFile != itsFileSeqPtr->end(); ++itFile){
                if(*itFile !=NULL){
                cout <<"Filename::" << ((*itFile)->fileName);
                cout <<" path::" << ((*itFile)->path) << endl;
                }
    		 }
    	}
    
    void RunIt(){
    	ClassRoom	kinder;
    	string tempy = "steve";
    	kinder.Pu........(&tempy,20,0);
    	string tempy2 = "john";
    	kinder.Pu........(&tempy2,25,1);
    	string tempy3 = "danny";
    	kinder.Pu........(&tempy3,30,6);
    	
        kinder.PushFile("dude","man");
        kinder.PushFile("dudemeis","manny");
        kinder.PushFile("dudey","manmeister");
        kinder.PushFile("miesterdude","manman");
        kinder.PushFile("dudeman","mandude");
        kinder.printIt();
    }
    
    
    int main() {
    	int i =0;
    	while(i< 10){
    		RunIt();
    		//sleep(1);
    		cout << "____________________________________" << endl;
    		i++;
    	}
    	cout << "end of main!" << endl;
    }

  2. #2
    Registered User
    Join Date
    Dec 2009
    Posts
    41
    Thats odd. the Pu........() method came up above as pu..........., replace those.

  3. #3
    Registered User
    Join Date
    Oct 2006
    Posts
    2,588
    it looks like your code is getting filtered by the forum's keyword matching system. can you change the name of that function to something the forum won't care about?

  4. #4
    Registered User hk_mp5kpdw's Avatar
    Join Date
    Jan 2002
    Location
    Northern Virginia/Washington DC Metropolitan Area
    Posts
    3,806
    Get rid of the char* struct elements and switch to std::string containers. You're making things needlessly complicated.




    The std::string container requires the <string> header, not the <string.h> header.




    Do not put using namespace std; declarations in header files. If necessary, explicitly qualify any elements within the header with std:: that need them for the program to compile.




    There doesn't seem to be a need for all those pointers except perhaps as an experiment. Is this a requirement? You could easily get rid of those pointers and not have to worry about what's in the destructor.

    If you really wanted all those pointers, switching the vector elements from raw pointers to some type of smart pointer and implementing a destructor for the Person/FileEntry structs would greatly simplify things. You'd probably only need to call delete on the class data members and be done with it.
    Last edited by hk_mp5kpdw; 02-01-2012 at 03:02 PM.
    Elysia likes this.
    "Owners of dogs will have noticed that, if you provide them with food and water and shelter and affection, they will think you are god. Whereas owners of cats are compelled to realize that, if you provide them with food and water and shelter and affection, they draw the conclusion that they are gods."
    -Christopher Hitchens

  5. #5
    Registered User
    Join Date
    Dec 2009
    Posts
    41
    All the char *s, pointers, and everything is part of a much larger( a couple million lines) program that I am experimenting for. I pulled out the parts i wanted to mess around with, in this case the setup of the vectors to be deleted. The only part that I can really change is the destructors. doing it this way makes it much easier to really see the effects a particular object has on memory.

  6. #6
    Registered User whiteflags's Avatar
    Join Date
    Apr 2006
    Location
    United States
    Posts
    7,761
    Quote Originally Posted by somekid413 View Post
    doing it this way makes it much easier to really see the effects a particular object has on memory.
    In what way? That's interesting, because you should be able to understand everything about memory without pointers.

    While it is true that most of the memory your program will use will come from the free store, there is no reason to put STL containers on the free store. They already use it to store whatever they contain. Plus, in C++, memory management isn't supposed to be a big chore.
    Last edited by whiteflags; 02-01-2012 at 04:31 PM.

  7. #7
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,308
    I'm sorry but you are doing it utterly wrong. It's so blatently wrong it's apalling.
    Code:
     *      Author:
    Blank huh... Yeah I wouldn't be caught putting my name against that either!

    In reality you will never come across a case where it makes sense to have a pointer to a std container as a member of a class. It also does not make sense to store pointers to these very simple objects within the class. It even makes no sense for the members within Person to be pointers, or the ofstream in ClassRoom. You've gone completely mad with pointers.

    "doing it this way makes it much easier to make sure everything has a huge negative impact on memory usage and execution speed."
    FTFY!

    You know how they say "C lets you shoot yourself in the foot and C++ allows you to re-use the bullets"?
    Well in your case you're using a rapid-fire chaingun that shoots cannonballs, the trigger is taped down, and your foot is in point blank range.

    If the aim here is to do the stupidest thing possible, that will cause the most problems, then you're well on your way to success.

    I look forward to reading about this project's failure on theDailyWTF.com in the surely not-too-distant future.

    Last but not least, the best way to destruct a vector is:
    Code:
    // this code block intentionally left blank
    Stop me when I'm getting through...
    Last edited by iMalc; 02-02-2012 at 12:09 AM.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  8. #8
    msh
    msh is offline
    Novice
    Join Date
    Jul 2009
    Posts
    568
    Quote Originally Posted by iMalc View Post
    I look forward to reading about this project's failure on theDailyWTF.com in the surely not-too-distant future.
    I don't think there's a way to make this funny... :S
    Disclaimer: This post shows my ignorance at the time of its making. I claim ownership of but not responsibility for all errors in it. Reference at your own peril.

  9. #9
    Registered User
    Join Date
    Dec 2009
    Posts
    41
    yea I recognized that it wasn't a great way to implement the containers.

    The fact of the matter is I recently started working for a big company, who's software you probably use, and probably have been using for years. They use software to point out memory leaks in their code, which Identified that a vector pointer to pointers was not getting freed and the memory was being leaked. I'm not saying this to pretend I'm smart. I'm clearly a novice developer, just trying to come up with the best solution to a problem.

    I replicated the container in a little bit of demo code and ran it with valgrind to test for mem leaks and In both cases, its really my only option for fixing this as far as I can tell.
    Last edited by somekid413; 02-02-2012 at 08:12 AM.

  10. #10
    Registered User whiteflags's Avatar
    Join Date
    Apr 2006
    Location
    United States
    Posts
    7,761
    The reason we find this so ridiculous is because you're triplicating the amount of work you need to do.

    Code:
    // more work to create ...
    vector<Person*> *v;
    v = new vector<Person*>();
    v->push_back(new Person("Bob", 17, 1));
    v->push_back(new Person("Alice", 16, 2));
    v->push_back(new Person("Charlie", 16, 3));
    
    // more work to use ...
    for ( size_t i = 0; i < v->size(); i++) {
        cout << (*v)[i]->getName();
    }
    
    // more work to clean up after ...
    for ( size_t i = 0; i < v->size(); i++ ) {
        delete (*v)[i];
    }
    delete v;
    I really don't care if you use valgrind to make this sort of code okay, valgrind is not a reason to make pointers your only use of storage. STL containers clean up after themselves and broken standard libraries (in such a spectacular fashion) are nonexistent.

  11. #11
    Frequently Quite Prolix dwks's Avatar
    Join Date
    Apr 2005
    Location
    Canada
    Posts
    8,048
    If your aim is to write better code, you should do as others in this thread have been suggesting and refactor your example (and the original codebase you're working on) so that it doesn't dynamically allocate pointers to STL containers like vectors. This has a good chance of fixing whatever memory leaks you have. I'm not sure if you know what an std::vector actually contains -- it's basically a size, an allocated size, and a pointer to where the data actually is in memory. So when you dynamically allocate a vector yourself it's like having
    Code:
    struct Vector {
        int size, allocated_size;
        char *data;
    };
    
    Vector *v = new Vector(); // first level of indirection
    v->data = new char[1];  // second level of indirection
    v->size = v->allocated_size = 1;
    v->data[0] = 'x';
    You have one level of indirection to the vector, which itself only stores a little more information before invoking yet another level of indirection. This doesn't really make sense. The vector class was designed to be allocated on the stack or to exist as a member variable of your class.

    Of course, you may be dealing with a lot of code that uses this unsavoury style and in which case your goal might simply be to fix a memory leak. It shouldn't be necessary to refactor the code just to interpret Valgrind's output. I usually run valgrind with "-v --leak-check=full --show-reachable=yes", I think the "leak-check" option will give fuller stack traces from leaked blocks ("show-reachable" shows leaks stored in static contexts, you may not care about that). Valgrind also has an extremely useful feature where you can record the leaks from one run, then create a suppressions file and re-run with very clean output. Useful if you're experimenting and just want one leak to go away.

    Try not to do memory management yourself. You can mess up easily and it's not always simple to hunt leaks down. Instead of using pointers, use the stack or create members inside your class; if you must use pointers, consider std::auto_ptr or the much nicer boost::shared_ptr from Boost, which has also made its way into the new version of C++ (C++11). Might not be useful in this case but something to keep in mind for the future, and for your own projects. And it's not that hard to write yourself.
    Code:
    template <typename Type>
    class AutoDeleter {
    private:
        Type *variable;
    public:
        AutoDeleter(Type *var) : variable(var) {}
        ~AutoDeleter() { delete variable; }
    
        const Type *get() const { return variable; }
        Type *get() { return variable; }
    };
    Then store AutoDeleters instead of pointers, or keep around a vector of AutoDeleters (std::vector<AutoDeleter>) in your class which will automatically delete the class's other variables when it's destructed. That's not really a standard way of doing things, but it's less intrusive. Anyway, I'm sure you can see there are lots of ways to attack the problem of memory-management, and pretty much the worst way is to make all your variables pointers and free them yourself.
    dwk

    Seek and ye shall find. quaere et invenies.

    "Simplicity does not precede complexity, but follows it." -- Alan Perlis
    "Testing can only prove the presence of bugs, not their absence." -- Edsger Dijkstra
    "The only real mistake is the one from which we learn nothing." -- John Powell


    Other boards: DaniWeb, TPS
    Unofficial Wiki FAQ: cpwiki.sf.net

    My website: http://dwks.theprogrammingsite.com/
    Projects: codeform, xuni, atlantis, nort, etc.

  12. #12
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    23,007
    Quote Originally Posted by dwks View Post
    Try not to do memory management yourself. You can mess up easily and it's not always simple to hunt leaks down. Instead of using pointers, use the stack or create members inside your class; if you must use pointers, consider std::auto_ptr or the much nicer boost::shared_ptr from Boost, which has also made its way into the new version of C++ (C++11). Might not be useful in this case but something to keep in mind for the future, and for your own projects. And it's not that hard to write yourself.
    If you have C++0B, it is better to use std::unique_ptr rather than std::auto_ptr.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  13. #13
    'Allo, 'Allo, Allo
    Join Date
    Apr 2008
    Posts
    612
    Quote Originally Posted by dwks View Post
    And it's not that hard to write yourself.
    Judging from the code, it's a bit harder than you thought What you have there is ok for simple stack allocation, but it'd need a proper copy constructor to be usable in a vector. The sentiment still stands though.

  14. #14
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    23,007
    The class really should disable copy constructor and assignment. If possible, implement move constructor and move assignment.
    But then we basically we a unique_ptr.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Converting string vector to integer vector
    By CPlus in forum C++ Programming
    Replies: 4
    Last Post: 05-08-2010, 06:43 AM
  2. including templatized vector and vector
    By -EquinoX- in forum C++ Programming
    Replies: 1
    Last Post: 11-22-2009, 04:46 AM
  3. Vector of pointers to vector of objects
    By Litz in forum C++ Programming
    Replies: 10
    Last Post: 11-06-2009, 03:29 PM
  4. Simple Question memset(vector<int>, 0, sizeof(vector<int>))
    By HyperShadow in forum C++ Programming
    Replies: 6
    Last Post: 12-10-2007, 04:56 PM
  5. connect vector<int> and vector<char>
    By hdragon in forum C++ Programming
    Replies: 1
    Last Post: 04-27-2006, 02:16 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21