2

I have a class which allocates memory on the heap and then the destructor frees it. My copy constructor is never being called for some reason and I do not understand why. Here is my implementation:

 AguiBitmap::AguiBitmap( const AguiBitmap &bmp )
    {

        this->nativeBitmapPtr = al_clone_bitmap(bmp.nativeBitmapPtr);
    }

    AguiBitmap::AguiBitmap( char *filename )
    {

        if(!filename)
        {
            nativeBitmapPtr = 0;
            return;
        }

        nativeBitmapPtr = al_load_bitmap(filename);

        if(nativeBitmapPtr)
        {

            width = al_get_bitmap_width(nativeBitmapPtr);
            height = al_get_bitmap_height(nativeBitmapPtr);
        }
        else
        {
            width = 0;
            height = 0;
        }
    }




    ALLEGRO_BITMAP* AguiBitmap::getBitmap() const
    {
        return nativeBitmapPtr;
    }

However, When I do something like:

AguiBitmap bitmap;
bitmap = AguiBitmap("somepath");

The copy constructor code is never called (set a breakpoint). And therefore, my issue of having an invalid pointer in the reconstructed object from the temporary object becomes invalid when the temporary one is destroyed.

What do I do to get my copy constructor to be called?

Thanks

jmasterx
  • 52,639
  • 96
  • 311
  • 557

6 Answers6

12

That bit of code wont invoke the copy constructor - it invokes the assignment operator (or the copy-assignment operator):

// a helper `swap` function
void AguiBitmap::swap(AguiBitmap& a, AguiBitmap& b)
{
    using std::swap;  // enable the following calls to come from `std::swap`
                      // if there's no better match

    swap(a.nativeBitmapPtr, b.nativeBitmapPtr);
    swap(a.width, b.width);
    swap(a.height,b.height);
}

AguiBitmap::AguiBitmap& operator=( const AguiBitmap &rhs )
{
    // use copy-swap idiom to perform assignment
    AguiBitmap tmp(rhs);

    swap( *this, tmp);
    return *this;
}

Also note that your copy constructor is incomplete, since the height and width members aren't being copied:

width = bmp.width;
height = bmp.height;
Michael Burr
  • 333,147
  • 50
  • 533
  • 760
  • I don't think this swapping will work the way it is. First, you should put a `using std::swap` into that `swap()` function of yours in order to pick up `std::swap()`. Second, in the assignment operator, either invoke your class member `swap(tmp)` or (better) add a specialization/overload for the free `swap()` function. – sbi Oct 20 '10 at 13:33
  • @sbi: you're right that there needs to be a `using std::swap` in there. And what you describe is what the assignment operator is doing - the 'helper' `AguiBitmap::swap()` is intended to be a static member of the `AguiBitmap` class. If the preference is to make it a non-static member that only takes the 'other' argument or an explicit specialization of the templated `std::swap` that could certainly be done instead. – Michael Burr Oct 20 '10 at 16:42
  • 2
    That swap function should be a friend, and you should take the parameter for `operator=` by value. Since you're making a copy anyway, there's no point doing it inside the function. Doing it in the parameter list allows the compiler to elide it with temporaries. – GManNickG Oct 20 '10 at 16:54
  • 1
    Yes, I misread the function, I only saw the `AguiBitmap::` prefix. But now I see that it's actually worse than I thought. If you put this `AguiBitmap` as a data member into another class, you won't be able to swap it the way itself swaps its own members. As GMan said, `swap()` needs to be a non-member; either a `friend` or calling a member `AguiBitmap::swap(AguiBitmap&)`. – sbi Oct 20 '10 at 19:16
7
AguiBitmap("somepath");

will invoke:

AguiBitmap::AguiBitmap( char *filename )

and the assignment will invoke the assignment operator

to invoke your copy constructor, do this:

AguiBitmap bitmap;
AguiBitmap anotherBitmap(bitmap)
Tom
  • 908
  • 1
  • 6
  • 14
3

Your code invokes the assignment operator, not the copy constructor. The copy constructor could be invoked for this code

AguiBitmap bitmap = AguiBitmap("somepath");

but the compiler might just as well optimize this and execute the equivalent of this instead:

AguiBitmap bitmap("somepath");

However, per the Rule of Three, when you have a copy constructor, you are very likely to need an assignment operator (and a destructor), too, anyway. So you should implement one.
See this answer for an idiomatic way to implement assignment on top of copy-construction and destruction.

Community
  • 1
  • 1
sbi
  • 219,715
  • 46
  • 258
  • 445
  • 4
    That code invokes the `AguiBitmap(char *filename)` constructor, not the copy constructor. – sje397 Oct 20 '10 at 12:55
2

Sorry, thats an assignment, not a copy

Chris Becke
  • 34,244
  • 12
  • 79
  • 148
  • 1
    What do I do then to reallocate the memory? Because the destructor fails when the temporary object dies. – jmasterx Oct 20 '10 at 12:46
2

You can implement operator=. To avoid code duplication, use your copy constructor to implement it (since they should do pretty much the same thing). This will result in the copy constructor being called, if indirectly.

This question covers how to implement this stuff safely and efficiently.

Community
  • 1
  • 1
sje397
  • 41,293
  • 8
  • 87
  • 103
0

You need something like this:

AguiBitmap &AguiBitmap::operator=(const AguiBitmap &guiBitmap)
{   
    if(&guiBitmap == NULL)
    {
      //throw Exception
    }

    if(&guiBitmap != this)
    {
      //do the copying here
       this->nativeBitmapPtr = al_clone_bitmap(guiBitmap.nativeBitmapPtr);
      //...
    }

    return *this;
}
AudioDroid
  • 2,292
  • 2
  • 20
  • 31
  • Nobody normal would write such awful assign operator. And the 2nd if is only needed in some special cases. – BЈовић Oct 20 '10 at 13:55
  • @VJo Can you maybe send me a link to an example of a less "awful assign operator"? The 2nd if is only needed in the "special case" where you don't want to run through the whole "copy part" when the two objects are the same anyway, or am I wrong? – AudioDroid Oct 20 '10 at 21:59
  • http://www.parashift.com/c++-faq-lite/assignment-operators.html explains the assignment operator. It all depends on what you want to do. 1) the reference has to point to an object (otherwise you have an UB) 2) you are right about self-check – BЈовић Oct 21 '10 at 07:39
  • hm...okay, if I look at that link mentioned above, I find my exact code example under [12.3] - "2. ... equivalently:" so I'm still a bit puzzled why this is "awful". Only difference I find is the first if-clause. That is to prevent a memory leak, if somebody does something like: TTestData *pDataA = new TTestData(); TTestData *pDataB = NULL; *pDataA = *pDataB; //BAD => memory leak – AudioDroid Oct 21 '10 at 09:47
  • That is not a memory leak, but an undefined behaviour. Dereferencing a NULL pointer is UB. As for self check - in many cases you do not need it, and it increases the code complexity. – BЈовић Oct 21 '10 at 12:22
  • Alright, thanks for clarification on the undefined behaviour. And I agree on that you don't always need a self check. And yes, it does increase code complexity. Personally I'd rather be on the save side though and therefore would only omit it, when I need fast running code and I triple checked that I don't need it. Oh well... – AudioDroid Oct 21 '10 at 13:15