C++ Adding Objects with pointers into Vector

Question

I have a vector that adds objects that contain SDL_Surface pointers as data members which means i have to use a copy constructor to implement a deep copy for the pointers. The object frees the surfaces(pointers) in the destructor and this is where the problem occurs. At the moment the object is added into the vector(by pressing a button) the program crashes but when i take away the SDL_FreeSurface(surface) from the destructor(memory leak) the program doesnt crash when i add the object to the vector. How do I properly add the objects into the vector? Some may think the problem is that the destructor is trying to delete dangling pointers but the error occurs upon the creation of the object in the vector.

class Block{

  public:

     Block(int x, int y, MediaFunctions &M_Functions);

     Block(const Block& source);

    ~Block();

  private:


    SDL_Surface *block_surface_names;
    SDL_Surface *block_surface_hours;

    SDL_Surface *block_names_detected;
    SDL_Surface *block_hours_detected;

    SDL_Rect     block_rect_names;
    SDL_Rect     block_rect_hours;


    };



    ////////////////////

   Block::Block(int x, int y, MediaFunctions &M_Functions){

      block_surface_names  = M_Functions.LoadOptimizedImage("block_names.png");
      block_surface_hours  = M_Functions.LoadOptimizedImage("block_hours.png");

      block_names_detected = M_Functions.LoadOptimizedImag("block_names_detected.png");
      block_hours_detected = M_Functions.LoadOptimizedImag("block_hours_detected.png");




      block_rect_names.x = x;
      block_rect_names.y = y;
      block_rect_names.w = block_surface_names -> w;
      block_rect_names.h = block_surface_names -> h;


      block_rect_hours.x = block_rect_names.x + block_rect_names.w;
      block_rect_hours.y = block_rect_names.y;
      block_rect_hours.w = block_surface_hours -> w;
      block_rect_hours.h = block_surface_hours -> h;



     }

     //copy
     Block::Block(const Block& source) 
     {
     block_surface_names  = source.block_surface_names;
     block_surface_hours  = source.block_surface_hours;

     block_names_detected = source.block_names_detected;
     block_hours_detected = source.block_hours_detected;

     }


    Block::~Block(){
     //having this is necessary obviously- crashes program
     //removing this causes the program not to crash

     SDL_FreeSurface(block_surface_hours);
     SDL_FreeSurface(block_surface_names);

     SDL_FreeSurface(block_hours_detected);
     SDL_FreeSurface(block_names_detected);

    }


    //where the object with SDL_FreeSurface() in the dtor is added to vector - crash!
   void Control::HandleEvents(SDL_Event &event, MediaFunctions &M_Functions){

       if(event.type == SDL_KEYDOWN){
           if( event.key.keysym.sym == SDLK_a )

            //append a block instance using copy constructor
            BlockVector.push_back(Block (Block(100,100, M_Functions) ) );

       }

     }

score 2 · Accepted Answer · edited May 23 '17 at 12:27

2

Your code:

BlockVector.push_back(Block (Block(100,100, M_Functions) ) );

looks very suboptimal, it creates unnecessary copies of data that looks like requires some time to load, I mean this png loading in Block::Block().

The best what you can do is to make BlockVector to be:

 std::vector<boost::shared_ptr<Block>> blocks;

this way you wont need to make unnecessary copies of Block. Otherwise you would need to add reference counting to your SDL_Surface* pointers in Block class, this can also be done with shared_ptr and custom deleter (for that look here: make shared_ptr not use delete).

edited May 23 '17 at 12:27

Community

1
1

answered Oct 15 '12 at 23:57

marcinj

48,511
9
79
100

`shared_ptr`? Why would you add that much overhead? Why not show him how to make `Block` movable? – Mooing Duck Oct 16 '12 at 00:04
would having shared_ptr fix the crash at all? – lambda Oct 16 '12 at 00:06
@MooingDuck yes movable is great idea here – marcinj Oct 16 '12 at 00:09
@Unit978 it will fix this crash, the problem is that your temporary objects delete pointers which are being copied to objects in vector. You dont give exact callstack where crash happend, but this might be because of access to such dangling pointer, or because of later destruction of such pointers – marcinj Oct 16 '12 at 00:14

Mooing Duck · Answer 2 · 2012-10-16T01:34:47.820

0

Copy constructors should do deep copies, but yours doesn't. Luckily, you don't actually need a copy constructor at all, just a move constructor.

 Block::Block(Block&& source) 
 {
 block_surface_names  = source.block_surface_names;
 block_surface_hours  = source.block_surface_hours;
 source.block_surface_names = NULL;
 source.block_surface_hour = NULL;

 block_names_detected = source.block_names_detected;
 block_hours_detected = source.block_hours_detected;
 source.block_names_detected = NULL;
 source.block_hours_detected = NULL;
 }

Only vaguely related to your issue:

BlockVector.push_back(Block (Block(100,100, M_Functions) ) );

This makes a Block, then makes a copy of that Block, then pushes a copy of that block onto the vector. However, it's possible to simply make the Block directly in the vector, with this code:

BlockVector.emplace_back(100, 100, M_Functions);

If you don't have a C++11 enabled compiler, you're best off using a vector of boost::shared_ptr, which is slower and more complex than this code, but would also solve the problem. In either case, the Block class should have a deleted(or undefined) copy constructor.

edited Oct 16 '12 at 01:34

answered Oct 16 '12 at 00:06

Mooing Duck

64,318
19
100
158

what would be the negative aspects of placing the object directly into the vector? – lambda Oct 16 '12 at 00:12
@Unit978: Only the normal negative aspects to a `vector`, like pointers to `Block`s could be invalidated when you insert more data in the `vector`, but nothing to do with `Block`. Other than that, nothing. – Mooing Duck Oct 16 '12 at 00:13
*"and that pretty much solves the issue"* -- No it doesn't. He also needs to null the pointers of the incoming object. – Benjamin Lindley Oct 16 '12 at 01:01
@BenjaminLindley: You're right of course, I got used to `unique_ptr` – Mooing Duck Oct 16 '12 at 01:34
Should use that then. Then there's no need for a user defined move constructor or destructor, just a custom deleter. – Benjamin Lindley Oct 16 '12 at 01:46

C++ Adding Objects with pointers into Vector

2 Answers2