losing pointers with destructor

This is a discussion on losing pointers with destructor within the C++ Programming forums, part of the General Programming Boards category; I wrote a class to stat directory contents, derived from vector. One of the methods is "subdirs" -- it returns ...

  1. #1
    spurious conceit MK27's Avatar
    Join Date
    Jul 2008
    Location
    segmentation fault
    Posts
    8,300

    losing pointers with destructor

    I wrote a class to stat directory contents, derived from vector. One of the methods is "subdirs" -- it returns a vector<directory>.

    Code:
    vector<directory> directory::subdirs() {
    	vector<directory> retv;
    	iterator it = begin();
    	while(it != end()) {
    		if (ftype(it) == 'd' && (*it).read) {
    			char dpath[PATH_MAX+1];
    			if ((*it).link) strcpy(dpath,(*it).link);
    			else sprintf(dpath, "%s/%s", path.c_str(), (*it).entry.d_name);
    			directory cur(dpath, stype, restat_links);
    			retv.push_back(cur);
    		}
    		it++;
    	}
    	return retv;
    }
    So push_back copies cur into the vector, then the destructor is called on cur because of scope. This causes a problem (can be a bus error or data corruption):

    Code:
    directory::~directory() {
    /*	vector<fileinfo>::iterator it = begin();
    	while (it != end()) {
    		if ((*it).link) free((*it).link);
    		it++;
    	}
    	if (stype) free(stype);*/
    }
    Commenting out that stuff solves the problem, I am assuming because of the pointers. This is my first week with C++, and there's a few hazy possible solutions in my mind,* but I wanted to ask and see if there is a strategy here more obvious to the experienced.

    Thanks in advance!

    * eg, stype and link are pointers to malloc'd C-strings, I could make them normal (copyable) arrays.
    Last edited by MK27; 02-27-2010 at 07:30 PM.
    C programming resources:
    GNU C Function and Macro Index -- glibc reference manual
    The C Book -- nice online learner guide
    Current ISO draft standard
    CCAN -- new CPAN like open source library repository
    3 (different) GNU debugger tutorials: #1 -- #2 -- #3
    cpwiki -- our wiki on sourceforge

  2. #2
    and the hat of sweating
    Join Date
    Aug 2007
    Location
    Toronto, ON
    Posts
    3,545
    Quote Originally Posted by MK27 View Post
    I wrote a class to stat directory contents, derived from vector.
    Wait a sec... You class derives from std::vector? That's a no-no. STL containers aren't meant to be base classes, since they don't have virtual destructors. If you want to make use of a vector in your class, you should have a vector member variable.

    Quote Originally Posted by MK27 View Post
    Code:
    directory::~directory() {
    /*	vector<fileinfo>::iterator it = begin();
    	while (it != end()) {
    		if ((*it).link) free((*it).link);
    		it++;
    	}
    	if (stype) free(stype);*/
    }
    Why are you using free() in a C++ program? You should be using new & delete.
    "I am probably the laziest programmer on the planet, a fact with which anyone who has ever seen my code will agree." - esbo, 11/15/2008

    "the internet is a scary place to be thats why i dont use it much." - billet, 03/17/2010

  3. #3
    Registered User NeonBlack's Avatar
    Join Date
    Nov 2007
    Posts
    435
    Quote Originally Posted by cpjust View Post
    Why are you using free() in a C++ program? You should be using new & delete.
    And std::string

    And iostream.

    And you could avoid fugliness such as (*it).link by using the arrow it->link

    And you could make those while loops into for loops.

    And... No, that's it for now.
    I copied it from the last program in which I passed a parameter, which would have been pre-1989 I guess. - esbo

  4. #4
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,308
    Your problem almost certainly comes from not following the rule of three:
    http://en.wikipedia.org/wiki/Rule_of...2B_programming)
    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"

  5. #5
    spurious conceit MK27's Avatar
    Join Date
    Jul 2008
    Location
    segmentation fault
    Posts
    8,300
    Quote Originally Posted by iMalc View Post
    Your problem almost certainly comes from not following the rule of three:
    http://en.wikipedia.org/wiki/Rule_of...2B_programming)
    Yeah, that is the ticket, thanks much! I had this thot this morning when I got up, actually -- why not write a copy op that includes push_back instead of just using push back? This way I can keep my pointers, which are more handy in this instance than strings or arrays. Actually that link should be:
    http://en.wikipedia.org/wiki/Rule_of...B_programming)

    Quote Originally Posted by NeonBlack View Post
    And std::string
    And iostream.
    I am. The reason I used C-strings and pointers for those two is that it is

    1) more convenient in the context of their use
    2) both pointers are usually NULL, which again provides a convenience

    And you could make those while loops into for loops.
    No, I prefer the while loop thanks.

    Quote Originally Posted by cpjust View Post
    Wait a sec... You class derives from std::vector? That's a no-no. STL containers aren't meant to be base classes, since they don't have virtual destructors.
    I'm aware of that (thanks for the warning), however, that would mean doing this, which is just stupid:
    Code:
    directory mydir(..);
    mydir.filevector[x] 
    iterator.filevector.etc
    Rather than just being able to refer to the files as "mydir[x]" or directly with an iterator -- since the vast majority of operations on the object are on it's members. Esp when it becomes "object.membervector.method" -- bah! Esp. if I am further returning a vector or tree of such directory objects.

    So to avoid the virtual destructor problem I just have to refrain from doing this (?):
    Code:
    vector<fileinfo> *dir = new directory(...);


    Why are you using free() in a C++ program? You should be using new & delete.
    For some reason I though new and delete were for objects only. I'll probably change this, but does it matter for any particular reason?
    Last edited by MK27; 02-28-2010 at 07:47 AM.
    C programming resources:
    GNU C Function and Macro Index -- glibc reference manual
    The C Book -- nice online learner guide
    Current ISO draft standard
    CCAN -- new CPAN like open source library repository
    3 (different) GNU debugger tutorials: #1 -- #2 -- #3
    cpwiki -- our wiki on sourceforge

  6. #6
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,910
    Quote Originally Posted by MK27
    I'm aware of that (thanks for the warning), however, that would mean doing this, which is just stupid:
    I do not see what is so stupid about it.

    Quote Originally Posted by MK27
    Rather than just being able to refer to the files as "mydir[x]" or directly with an iterator -- since the vast majority of operations on the object are on it's members. Esp when it becomes "object.membervector.method" -- bah! Esp. if I am further returning a vector or tree of such directory objects.
    It sounds like you are programming on a wrong level of abstraction. What exactly is your directory class supposed to model, and what is its current class definition?

    Quote Originally Posted by MK27
    So to avoid the virtual destructor problem I just have to refrain from doing this (?):
    That is one way, but it really would be better if you did not inherit publicly.

    Quote Originally Posted by MK27
    For some reason I though new and delete were for objects only. I'll probably change this, but does it matter for any particular reason?
    It does not matter, though it may be better to be consistent since you might mix up when a type is a POD type and when it is not.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  7. #7
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    4,332
    however, that would mean doing this, which is just stupid:
    No. You could export the member as a public interface, but you wouldn't have to do that. If you bothered to write the code for the interface, you probably wouldn't have to change any client source.

    since the vast majority of operations on the object are on it's members
    O_o

    Every operation on every object is ultimately on its members.

    So to avoid the virtual destructor problem I just have to refrain from doing this (?):
    Yes. However, you have to avoid any use of polymorphism, not just the destructor. (You could get away with static polymorphism, as templates, from the STL or such.)

    I'll probably change this, but does it matter for any particular reason?
    It makes your code brittle. If you always use new/delete changing from a simple type to a user defined type will not cause "Bad Things".

    Soma

  8. #8
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,910
    Quote Originally Posted by phantomotap
    Yes. However, you have to avoid any use of polymorphism, not just the destructor.
    Well, you cannot really use inheritance based polymorphism anyway since the base class has no virtual member functions.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  9. #9
    spurious conceit MK27's Avatar
    Join Date
    Jul 2008
    Location
    segmentation fault
    Posts
    8,300
    Quote Originally Posted by laserlight View Post
    I do not see what is so stupid about it.
    I don't see anything particularly smart about it, either.

    Quote Originally Posted by laserlight View Post
    It sounds like you are programming on a wrong level of abstraction. What exactly is your directory class supposed to model, and what is its current class definition?
    Could easily be (this is like C++ program #3). Phantomotap also implies a solution, "You could export the member as a public interface", not sure where that fits in. Anyway:
    Code:
    class directory : public vector<fileinfo> {
            public:
                    string path;
                    int error;              // errno from opendir; 0 when OK
                    directory(const char *path, char *stype = NULL, bool restat_links = true);
                    char ftype(iterator it);
                    vector<directory> subdirs();
                    ~directory();
            private:
                    DIR *dir;
                    char *stype;
                    bool restat_links;
                    bool statfile(); 
                    int dots (char *name);
    };
    Hopefully that is clearer. This is a "fileinfo":

    Code:
    typedef struct {
            string name;
            struct dirent entry;
            struct stat stat;
            char suf[16];
            bool read;              // have read permissions
            char *link;             // NULL unless symlink
    } fileinfo;
    So generally calls are of the form:
    Code:
    directory dir(...);
    dir[x].stat.st_size
    dir.filevector.stat.st_size just seems pointlessly clumsy.
    C programming resources:
    GNU C Function and Macro Index -- glibc reference manual
    The C Book -- nice online learner guide
    Current ISO draft standard
    CCAN -- new CPAN like open source library repository
    3 (different) GNU debugger tutorials: #1 -- #2 -- #3
    cpwiki -- our wiki on sourceforge

  10. #10
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,910
    Quote Originally Posted by MK27
    I don't see anything particularly smart about it, either.
    Yeah, it is just normal.

    Quote Originally Posted by MK27
    Could easily be (this is like C++ program #3). Phantomotap also implies a solution, "You could export the member as a public interface", not sure where that fits in. Anyway:
    You are effectively exposing the internals of your directory class. I would interpret "export the member as a public interface" as basically providing an interface by which a fileinfo object belonging to the directory object can be accessed, For example, you could overload operator[] to return a reference to a fileinfo object, and thus users of your class might write:
    Code:
    directory dir(...);
    dir[x].stat.st_size
    Furthermore, path should be private, then you provide a member function to access it; otherwise it may be the case that it is changed yet the directory path is not changed, thus making the directory object have inconsistent state.
    Last edited by laserlight; 02-28-2010 at 08:25 AM.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  11. #11
    spurious conceit MK27's Avatar
    Join Date
    Jul 2008
    Location
    segmentation fault
    Posts
    8,300
    Quote Originally Posted by laserlight View Post
    You are effectively exposing the internals of your directory class. I would interpret "export the member as a public interface" as basically providing an interface by which a fileinfo object belonging to the directory object can be accessed, For example, you could overload operator[] to return a reference to a fileinfo object, and thus users of your class might write:
    Code:
    directory dir(...);
    dir[x].stat.st_size
    Furthermore, path should be private, then you provide a member function to access it; otherwise it may be the case that it is changed yet the directory path is not changed, thus making the directory object have inconsistent state.
    Alright! Most of the actual vector operation are inappropriate as well (I don't want or need to create/delete files via the interface), so maybe I can make the whole vector private, accessed only by [] and an iterator.
    Last edited by MK27; 02-28-2010 at 08:41 AM.
    C programming resources:
    GNU C Function and Macro Index -- glibc reference manual
    The C Book -- nice online learner guide
    Current ISO draft standard
    CCAN -- new CPAN like open source library repository
    3 (different) GNU debugger tutorials: #1 -- #2 -- #3
    cpwiki -- our wiki on sourceforge

  12. #12
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,910
    Quote Originally Posted by MK27
    Alright!
    That said, I am not entirely sure if overloading operator[] is the right thing to do. It kind of makes sense to me, i.e., a directory entry is a file (or information about a file), but then using a named member function might be clearer.

    Oh, and if you do overload operator[], you should const overload too: provide a version that works when the directory object is const.

    Finally, you might want to take a look at Boost.Filesystem.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  13. #13
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    4,332
    Well, you cannot really use inheritance based polymorphism anyway since the base class has no virtual member functions.
    I was going to give you the benefit of doubt. I've changed my mind. No explanation for you. That was stupid. You either jumped in with some random quip without bothering to understand what I was saying or you truly have no idea what you're talking about.

    Phantomotap also implies a solution, "You could export the member as a public interface", not sure where that fits in.
    It's just one of many possible ways to have the interface you want without inheriting `std::vector<???>'.

    Soma

  14. #14
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,910
    Quote Originally Posted by phantomotap
    I was going to give you the benefit of doubt. I've changed my mind. No explanation for you. That was stupid. You either jumped in with some random quip without bothering to understand what I was saying or you truly have no idea what you're talking about.
    Frankly, other than the "no", I found your comment stupid, but I gave you the benefit of the doubt by providing my own explanation. Apparently, you are less charitable.

    EDIT:
    *sigh* I guess that I should not say things in anger, but your insinuation that I may "truly have no idea what (I am) talking about" really made me angry. Right, I see where you are coming from now. Why can you not understand that people might just see things differently from you? I was talking about polymorphism with respect to std::vector<fileinfo> as the base class (and that is what the question is about, as far as I can tell), so your comment did not make sense to me. So yes, it is true that I did not understand what you were saying, but your attitude towards this is far more hostile that it should be. Would you like me to simply ignore you in the future?
    Last edited by laserlight; 02-28-2010 at 09:10 AM.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  15. #15
    spurious conceit MK27's Avatar
    Join Date
    Jul 2008
    Location
    segmentation fault
    Posts
    8,300
    Quote Originally Posted by laserlight View Post
    That said, I am not entirely sure if overloading operator[] is the right thing to do. It kind of makes sense to me, i.e., a directory entry is a file (or information about a file), but then using a named member function might be clearer.

    if you do overload operator[], you should const overload too: provide a version that works when the directory object is const
    Unlikely that anyone but me will be using this -- but more generally, I don't mind having to pay attention and read docs and consider that "for the best". Thus: I don't feel a big need to bend over too far backward on the premise that the user won't bother.

    Also thinking the directory should always be const, is there a way to do that? (I guess keeping the vector private will have the same effect).

    Finally, you might want to take a look at Boost.Filesystem.
    Yeah, I noticed that. I am working on my own alternative, the "Hooch C++ libraries"... For some reason every time I approach a new language the first thing I want to do is tree the filesystem. Must be an i/o fetish.
    Last edited by MK27; 02-28-2010 at 09:04 AM.
    C programming resources:
    GNU C Function and Macro Index -- glibc reference manual
    The C Book -- nice online learner guide
    Current ISO draft standard
    CCAN -- new CPAN like open source library repository
    3 (different) GNU debugger tutorials: #1 -- #2 -- #3
    cpwiki -- our wiki on sourceforge

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. pointers to arrays
    By rakeshkool27 in forum C Programming
    Replies: 1
    Last Post: 01-24-2010, 06:28 AM
  2. sorting number
    By Leslie in forum C Programming
    Replies: 8
    Last Post: 05-20-2009, 04:23 AM
  3. Variable pointers and function pointers
    By Luciferek in forum C++ Programming
    Replies: 11
    Last Post: 08-02-2008, 02:04 AM
  4. API "Clean Up" Functions & delete Pointers :: Winsock
    By kuphryn in forum Windows Programming
    Replies: 2
    Last Post: 05-10-2002, 06:53 PM

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