3

I have a struct object that contains a pointer to an object.

struct Track
{
    KalmanFilter* kalmanFilter; //Kalman filter for this track
    ....
};

I instantiate the struct and pointer objects by

Track track;
track.kalmanFilter = new KalmanFilter(contours[contour].centroid);
....

I then check each frame if the track is still valid by

//Temporary vector to hold alive tracks
std::vector<Track> tempTracks;

for(...)
{
    //If track is valid save into temp tracks
    if(tracks[track].deadTime <= deadTime) tempTracks.push_back(tracks[track]);
}

//Save all temp tracks back into tracks - this removes dead tracks that were in tracks
tracks = tempTracks;

After a while the micro-controller i'm using runs out of memory, this is the only pointer in my code so i'm pretty sure the KalmanFilter pointer is not being deleted.

So far I have tried a destructor in the Track struct which causes my micro-controller to hard fault immediately.

//Destructor frees up Kalman filter memory when the Track object goes out of scope
~Track()
{
    if(kalmanFilter) delete kalmanFilter;
} 

and I have tried using a unique_ptr but I cannot get the code to compile, all the examples I have found instantiate the object being pointed to at the same time as instantiating the unique_ptr. In my case I don't think I can do this.

What is the correct way to handle a situation like this?

Thanks

EDIT

So based on the comments there seems to be two main points.

Allocate the memory in the constructor

I have rewritten my struct to be

struct Track
{
    Track(const Point& initialPosition) : kalmanFilter(initialPosition) {}
    ~Track() {}
    KalmanFilter kalmanFilter; //Kalman filter for this track
};

and I instantiate it by

Track track(contours[contour].centroid);

Is this correct?

Find a better way to delete the tracks

What is the best way to remove an object from a vector without messing up the indexing during the loop? Using an STL iterator?

I implemented the solution from @M.M that seems to work

tracks.erase(std::remove_if(tracks.begin(), tracks.end(), [&](Track const &t){return t.deadTime > deadTime;}));
12
  • 3
    Did you follow the rule of three? Commented Nov 9, 2015 at 21:18
  • DR; TL; You have to delete allocated memory before the object gets out of scope. Commented Nov 9, 2015 at 21:18
  • @101010 yes I can do this manually but I was hoping there was some automatic way of doing it. Either a deconstructor that works or a unique_ptr Commented Nov 9, 2015 at 21:21
  • You should actually follow @NathanOliver's advice. Don't manage memory allocation outside of a struct, even better encapsulate such behavior interned with a class, and don't leave it publicly. Even more better, use a standard c++ container or smart pointer, and leave memory de-/allocation to this member. Commented Nov 9, 2015 at 21:22
  • 1
    "all the examples I have found instantiate the object being pointed to at the same time as instantiating the unique_ptr." unique_ptrs can be assigned to if the right hand side is an r-value: myptr = std::make_unique<KalmanFilter>(contours[contour].centroid); this automatically deletes the previous contents of the unique pointer. Commented Nov 9, 2015 at 21:23

3 Answers 3

4

You can use a smart pointer:

struct Track
{
    std::unique_ptr<KalmanFilter> kalmanFilter;
....
};

This means that when the Track is destroyed, if the smart pointer is managing a pointer it will invoke delete on that pointer.

It could be assigned by:

track.kalmanFilter.reset(new KalmanFilter(contours[contour].centroid));

although it'd be preferable to have this done by the constructor of Track.


This change will render Track non-copyable (but still movable). This is a good thing because previously your code did not behave properly on being copied anyway, so now the compiler will catch it for you.

You will need to modify your erase-loop to stop making copies of the elements. The normal way to do this is to use vector::erase combined with std::remove_if (the latter moves the selected elements to the end, and erase erases elements off the end):

tracks.erase( std::remove_if( begin(tracks), end(tracks), 
    [&](Track const &t) { return t.deadTime > deadTime; } ), end(tracks) );

Required includes would be <memory> and <algorithm>.

Sign up to request clarification or add additional context in comments.

5 Comments

Thanks for answer, can you explain the last bit in more detail? Will that code remove tracks with a deadtime of <= deadtime or tracks with a deadtime of > deadtime?
The former. The function should return true for items to be removed, and false otherwise. I'll edit
OK thanks, I was comparing your solution to the wikipedia entry "v.erase( std::remove_if(v.begin(), v.end(), is_odd), v.end() );" Why does your solution no require v.end() after the function?
tracks.end() is needed as last parameter to tracks.erase(), otherwise only one element will be removed. @JosephRoberts did you verify that?
Thanks @A.S.H good to know. I didn't test properly yet, I need to check it gives me the behavior I want and play around with the tracks.erase function if not. Instantiating the Kalman filter in the tracks constructor does however definitely work.
1

The way you are eliminating dead tracks will cause many copies of each track to be made.

The destructor to one of your many copies will try to deallocate that memory while active copies are still using that kalman filter which can cause issues. Also, if you can't guarantee it is NULL before being assigned... could be a random pointer being deleted.

In this situation I'd use a shared pointer. But to be honest, I feel like there is a better overall structure for how you are storing/using the tracks, I'd have to think about it and know more about the implementation though.

Comments

-1

When you do push_back() you are creating a copy of the original Track object. Both the copy and the original object point to the same memory, and both at some point get destructed resulting in a double deallocation of the same address. Following the suggestions in the comments on better code organization will help avoid such bugs.

1 Comment

It'd be good to make your answer include suggestions, instead of directing people to comments (which may be deleted later, and shouldn't really include answers anyway). You can credit the comment author by mentioning their name if you like.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.