Re: simple vector question
* brendan.morris@gmail.com:
I have a complex class that I am storing in a vector. I seem to have a
memory leak and I think it's because I don't really know how to use
vectors properly. Not only that but my code is overly complex.
To reduce or avoid memory leaks:
* Replace raw pointers + direct usage of 'delete' with smart pointers
(e.g. boost::shared_ptr for objects in collections, std::auto_ptr
for transferring ownership).
* Replace direct usage of 'new T[n]' with std::vector.
* Make sure that remaining objects that do direct memory management
are not copied outside your control, by declaring a copy constructor
and an assignment operator. You can disable copying by making these
operations private or protected. Or you can take charge of copying.
* Make sure that objects that are destroyed polymorphically have
virtual destructor.
* Be clear about responsibilities and object ownership.
[snip]
I create create the tracks object by a loop
for (int i=0; i<num_Tracks; i+=1) {
tracks.push_back(new BmTrack(d,deltaT,sigma1,sigma2,track_total));
}
The size of the vector is constantly changing by removing elements from
within the vector. The way I'm dong it looks something like this.
BmTrack* t;
for(i=num_tracks-1; i>=0; i-=1) {
if(dead_track[i]) {
t = tracks.at(i);
delete t;
tracks.erase(tracks.begin()+i);
}
}
Am I doing something wrong here.
Maybe. The code shown doesn't update the 'dead_track' array: that may
bite you if this code is executed repeatedly, as you indicate it is,
with 'dead_track' surviving between invocations.
Also, the code shown doesn't update the 'num_tracks' variable, which may
or may not constitute a bug. Why not use 'tracks.size()' instead?
Caching is just optimization: better to achieve correctness first. ;-)
Also, but only relevant for efficiency, if the number of tracks to be
erased is proportional to the size of the vector, call it n, then this
is an O(n2) algorithm, each erase operation being O(n); perhaps it
would be better to use a std::list or std::map instead of a std::vector.
I don't have a copy or assignment method defined for BmTrack.
Having a copy constructor and an assignment operator can help track down
inadvertent copying, or, they can help you to take charge of copying.
And is it a bad idea to create the BmTrack* t variable outside of
my loop?
Just stylistically. To prevent variables being used in unintended
harmful ways, place the declaration of a variable in the innermost scope
possible (some advocate that a variable should preferentially be
declared where it's first used). However, this particular variable is
not needed at all for the code shown, and can simply be removed, and the
loop variable 'i' should, stylistically, be declared in the loop, like
'for( std::size_t i = tracks.size()-1; i >= 0; --i )'.
--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]