Could this turn out to be dangerous?

[yasar11732]

Assume this is destructor for a file class:

file::~file ()
{
    if (parent != NULL)
    {
        parent->removeChildFile(this); // parent is a pointer to a directory
    }
}

This is how remove child file is defined in directory class (directory and file are friend classes both ways.)

void directory::removeChildFile(file * child)
{
    files.erase(child); // files is a set<file *> and a member function of directory class
}

So far so good, but problematic part is, what would happen if I wrote another member function for directory class, which looks like this:

void directory::releasefiles ()
{
    // If you didn't keep references to
    // individual files that you created with new, this
    // method allow you to call delete on all of the child files.
    set<file *>::iterator pos;
    for (pos = files.begin(); pos != files.end(); ++pos)
    {
        delete (*pos);
    }
}

Problem with this code is (in my opinion), deleting a file object, would call its destructor, which in turn calls removeChildFile, which in turn modify "files" set, that I am iterating upon and doing so could cause undefined behaviour. On the other hand, since I am modifying only the parts that I already saw during iteration, this code could work properly (or not?). Well, I am not sure. What do you think?

[ciphermagi]

It doesn't look too terrible, but try running it through valgrind, and see if you notice any leaks.

[mik2718]

It looks bad to me, see here:

http://stackoverflow.com/questions/1636578/iterator-validity-after-erase-call-in-stdset

which contains

From standard 23.1.2
The insert members shall not affect the validity of iterators and references to the container, and the erase members shall invalidate only iterators and references to the erased elements.

Clearly, for a set, other iterators are not invalidated but the one pointing to the erased object is. Therefore it looks like the ++pos in your for loop will fail.

[yasar11732]

Hmm, interesting. I didn't know that. How about this one:

void directory::releaseFiles ()
{
    // If you didn't keep references to
    // individual files that you created with new, this
    // method allow you to call delete on all of the child files.
    set<file *>::iterator pos, temp;
    for (pos = files.begin(); pos != files.end();)
    {
        temp = pos;
        ++pos;
        delete (*temp);
    }
}

[clanmjc]

Just curious why are directory and file friend classes? From what you've posted they need not be. Secondly, why create a dependency on the File class to remove itself from the Directory class' set? Also, what if a File is not associated with a Directory (I don't know why that would be the case but what if?) you would not be able to use the File object because when it gets deconstructed it will try to remove itself from the Set. (Forget the fact that the set would not be able to find it in the container so in this implementation it wouldn't matter)

As mik2718 states, once the ++pos is invoked an assertion for "iterator not incrementable" will occur.

[yasar11732]

Just curious why are directory and file friend classes?

You are right. I was trying out something when I made them friends, forgot to remove friendship afterwards.

Secondly, why create a dependency on the File class to remove itself from the Directory class' set?

As soon as file gets deleted, parent directory would know about it. So, there won't be any misleading pointers in there. I thought those might cause segfaults. This is not a dependency because if statement in destructor checks to see if there is a parent directory or not. Destructor is actually a no-op if file doesn't have a parent directory. I guess my second post solves the problem of invalid iterator. Other than that, only problem that I could be able to find is that, objects created on the stack causes memory errors, as in:

// bunch of includes goes here...
int main(int argc, char * argv [])
{
    directory dir("foo",NULL);
    file myfile("bar",&dir); // for constructor see below
    dir.releaseFiles(); // error
    return 0;
}

// -- constructor used above:
file::file(string fname, directory * parentptr)
{
    name = fname;
    parent = parentptr;
    parent->addFile(this);
}

But this look like working fine:

int main(int argc, char * argv [])
{
    directory dir("foo",NULL);
    for (int i=0; i < 100; ++i)
    {
        new file("bar",&dir);
    }
    std::cout << "Before release\n";
    dir.listFiles(); // outputs name of the each file
    dir.releaseFiles(); // no error
    std::cout << "After release\n";
    dir.listFiles(); // no files
    return 0;
}

[clanmjc]

I guess what I was getting at was Destructors are usually used to deallocate memory and do other cleanup for a class object and its class members when the object is destroyed.

Now you or someone else has to be aware that File removes itself from Directory in it's deconstructor, since you know about it it's no big deal, but Joe Schmoe could come in and create some havoc that's all I was saying.

Take your objects created on the stack example. Since Directory is responsible for freeing the memory used for it's container, wouldn't it make more sense to have the Directory control the allocation as well?

directory dir("foo",NULL);
//new method
dir.AddFile(file("bar",&dir));


//... somewhere else
directory::AddFile(const File &newFile)
{
    //Directory now owns this memory and nobody else... it doesn't matter if this was stack or heap
     file * pFile = new File(newFile);
     files.insert(pFile);
}

Removing the adding of the file to the container from within the file itself as well as removal from the deconstructor.