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) ) );

       }

     }

Community · Accepted Answer · 2017-05-23 12:27:56Z

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, 2017 at 12:27

CommunityBot

11 silver badge

answered Oct 15, 2012 at 23:57

marcinj

50.3k10 gold badges85 silver badges103 bronze badges

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

4 Comments

Mooing Duck Over a year ago

shared_ptr? Why would you add that much overhead? Why not show him how to make Block movable?

lambda Over a year ago

would having shared_ptr fix the crash at all?

marcinj Over a year ago

@MooingDuck yes movable is great idea here

marcinj Over a year ago

@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

Mooing Duck · Accepted Answer · 2012-10-16 01:34:47Z

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, 2012 at 1:34

answered Oct 16, 2012 at 0:06

Mooing Duck

67.6k19 gold badges107 silver badges169 bronze badges

5 Comments

lambda Over a year ago

what would be the negative aspects of placing the object directly into the vector?

Mooing Duck Over a year ago

@Unit978: Only the normal negative aspects to a vector, like pointers to Blocks could be invalidated when you insert more data in the vector, but nothing to do with Block. Other than that, nothing.

Benjamin Lindley Over a year ago

"and that pretty much solves the issue" -- No it doesn't. He also needs to null the pointers of the incoming object.

Mooing Duck Over a year ago

@BenjaminLindley: You're right of course, I got used to unique_ptr

Benjamin Lindley Over a year ago

Should use that then. Then there's no need for a user defined move constructor or destructor, just a custom deleter.

Collectives™ on Stack Overflow

C++ Adding Objects with pointers into Vector

2 Answers 2

4 Comments

5 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

5 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related