tags:

views:

906

answers:

7

I encounter many times with code where std::vector::clear() of class member of type std::vector is called in constructor and destructor.

I don't see why it's required:

  1. constructor - the class member of type std::vector is empty by default, so no need to call clear().
  2. destructor - the class member of type std::vector will be destroyed as part of standard destruction of object contnaining it. As part of vector destruction all value objects containied in it will be destroyed (if it heap allocated pointers to memory, they should be deleted "manually"), so again no need to call clear().

Do I miss something?

+5  A: 

Nope, you're right. Unless there is some additional business in the constructor (or constructor of the base classes) that require that, but the chances are very low...

Later edit

In case of destructor, one of the most common mistakes I saw is that some people assume that clear method will also call delete for vectors of pointers (vector), which, of course, it is not the case

Cătălin Pitiș
+14  A: 

No, you're not missing anything. I suspect this is (harmless) voodoo programming, sort of like setting a pointer to null after freeing it, or randomly calling repaint/revalidate in GUI code. The programmer remembers that it helped with some sort of bug in the past and now adds it unnecessarily "just in case". Who knows, maybe it'll help. Voodoo.

John Kugelman
nice term, 'voodoo programming' :)
xtofl
Setting a pointer to null isn't voodoo, it helps greatly in debugging. Also prevents you from deleting the same pointer twice, but nobody ever does that - right?
Mark Ransom
If anything, you want to know that you deleted the same pointer twice. It is better to fail loudly (and find out about it) than accidentally succeed (masking a bug that will come back to bite you)
Greg Rogers
I like that saying, "better to fail loudly"
GMan
@Mark: setting a pointer member to NULL in a dtor, when it's about to disappear entirely? Because that's the context we're talking about.
MSalters
+12  A: 

From the sound of things, the people who wrote that code were the ones who missed something. The only time it would make sense to call clear() in a ctor or dtor would be in the middle of some other code. For example, a ctor might read in some data, process it, then read in more data. In such a case, it's probably faster to use a single container for the data as you read it in, and clear it each time, than to create a new container every iteration.

Jerry Coffin
+3  A: 
  1. It's COMPLETELY unnessecary to clear the contents of an stl container in a constructor
  2. It's unnessecary to clear the contents of an stl container in a destructor UNLESS the container contains a pointer. If the pointer has been created using new, it still needs to be deleted first. After that it still will not be nessecary to .clear the container.

Consider this :

#define BOOST_TEST_MODULE StlContainers
#define BOOST_LIB_DIAGNOSTIC

#include <boost/test/unit_test.hpp>
#include <boost/assign.hpp>
#include <boost/assign/list_of.hpp>
#include <boost/assign/std/vector.hpp>

#include <vector>

using namespace boost::assign;
using namespace std;

const vector<int> my_ints_vector = list_of(0)(1)(1)(2)(3)(5)(8)(13)(21)(34);

struct ScopedStruct1
{
     ScopedStruct1(const vector<int> & v) : m_v(v) {}
     ~ScopedStruct1() {}
    private :
     vector<int> m_v;
};

class A 
{ 
    public :
     A(int i) : m_i(i) {}
     ~A() {}
    private :
     int m_i;
};

struct ScopedStruct2
{
    ScopedStruct2() {}
    ~ScopedStruct2() { for(vector<A*>::iterator it = m_v.begin(); it != m_v.end(); ++it) delete *it; }

    vector<A*> m_v;
};

struct ScopedStruct3
{
    ScopedStruct3() {}
    ~ScopedStruct3() { /* no deletion */ }

    vector<A*> m_v;
};

BOOST_AUTO_TEST_CASE(StlContainer_storing_something_simple)
{
    ScopedStruct1 str(my_ints_vector);
}

BOOST_AUTO_TEST_CASE(StlContainer_storing_pointers_with_delete)
{
    ScopedStruct2 str;
    for(int i = 0; i < 10; i++)
     str.m_v.push_back(new A(i));
}

BOOST_AUTO_TEST_CASE(StlContainer_storing_pointers_without_delete)
{
    ScopedStruct3 str;
    for(int i = 0; i < 10; i++)
     str.m_v.push_back(new A(i));
}

Using boost's unit_test framework I've created 3 test cases. The unit_test framework is greate because it tracks memory leaks. You'll notice that the 1st and 2nd test case don't generate memory leaks, but the 3rd case does because the vector's contents are not deleted.

Maciek
A: 

Of course one has to call clear() or resize(0) or the equivalent say (std::_Destroy_range(...) in the destructor before deallocating.

Deallocation is done via allocator::deallocate which does NOT run any destructors. It just frees the memory.

clear() is equivalent to resize(0) which runs destructors on the first size() things in the allocated buffer

NOT just allocated pointers, file handles, held mutexes, all other recoverable resources held by the object. The destructors MUST be run. Before instantiation the template doesn't know that the destructor is trivial. If the destructor is trivial, THEN its gets optimized out AFTER instantiation

pgast
No, destructors are definitely called when a vector goes out of scope. Not calling destructors makes no sense. I suggest you examine your implementation carefully. You can test this by running something like this:#include <vector>#include <iostream>struct Test { ~Test() { std::cout << "Bye, world\n"; } }; int main() { std::cout << "Creating\n"; std::vector<Test> t(1); std::cout << "Created\n"; }
janm
Absolutely incorrect.
Drew Hall
I read him as discussing a clear call in ~vector() rather than the discussion of a clear call on a vector member instance variable of a user defined class - my misread.
pgast
A: 

The only case I can think of where it would be useful is where the order of destruction matters and the destructor wants to ensure that the objects in the vector are destroyed before something else.

Of course, it is better to structure code to not require that; however, it is a conceivable reason.

janm
A: 

Despite what what said so far, there's at least one scenario when an explicit call to clear in the destructor might be necessary.

Imagine the situation when the object being destroyed has several member subobjects, which require some specific order of destruction, i.e. the subobjects somehow depend on each other, and an incorrect order of their destruction will lead to undesirable results. As you probably know, the order of member subobject destruction (as well as member initialization) is determined by the order of members' declararion in class definition. So, one way to achive the proper order of destruction is to arrange the member declarations accordingly. However, firstly, this is not a very good solution maintenance-wise. Secondly, the desired order of destruction might depend on some run-time conditions. Thirdly, the desired order of destruction might contradict the desired oreder of initialization. This all means that it might not be possible (or wise) to command the proper order of destruction by re-arranging the declarations.

A reasonable approach in this case might be to clean-up some critical member subobjects manually, by calling their clean methods or such, until the destruction order dependency "disappears". I would guess, that maybe the code that you saw was trying to resolve to ordering problem by calling clean on a strategically selected vector subobject.

As for calling clean in constructor... I have no idea why anyone would do something like that.

AndreyT
Apperently you live in too reallistic word :) Every time I examined code, it tried to achive "clean" in constructor/destructor. As regards to situation you describe, I can hardly amagine it. Best should be done to avoid it and if not possible should be well commented
dimba
I might be wrong, but when the stl object is dying at an end of scope - for instance. It calls the destructor of the object within. If that said object is also an stl container it should clear itself up automatically too. Exceptional situation would arise with a pointer initiated by "new" - as usual
Maciek
@Maciek: Nobody argues with that. What I'm saying, again, that the order of that *automatic* cleanup might be unacceptable in some cases. In which cases you might have to do your own, custom ordered pre-cleanup before the automatic cleanup begins.
AndreyT