Thread: 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.

Originally Posted by MK27

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.

Originally Posted by MK27

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.

Originally Posted by cpjust

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.

Your problem almost certainly comes from not following the rule of three:
http://en.wikipedia.org/wiki/Rule_of...2B_programming)

Originally Posted by iMalc

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)

Originally Posted by NeonBlack

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.

Originally Posted by cpjust

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?

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.

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?

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.

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.

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.

Originally Posted by laserlight

I do not see what is so stupid about it.

I don't see anything particularly smart about it, either.

Originally Posted by laserlight

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.

Originally Posted by MK27

I don't see anything particularly smart about it, either.

Yeah, it is just normal.

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.

Originally Posted by laserlight

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.

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.

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?

Originally Posted by laserlight

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.