3

(The answer to my question involves copy constructors, but the copy takes place upon return from a function, not within a method call to another class. I actually saw the referenced possible duplicate, but did not infer from the copy made by vector::push_back that my function here also made a copy. Perhaps I should have.)

I am trying to understand the construction/destruction of automatic objects. I ran into some code that looked dubious to me, so I wrote my own version in an effort to understand it. In short, the original code included a function that returned an object that was local to the function (an automatic). That looked unsafe to me, so I wrote this program to explore it:

#include <stdio.h>

class Phantom
{
private:
    static int counter;
    int id;

public:
    Phantom()
    {
        ++counter;
        id = counter;
        printf("Phantom %d constructed.\n", id);
    };

    virtual ~Phantom()
    {
        printf("Phantom %d destructed.\n", id);
    };

    void speak()
    {
        printf("Phantom %d speaks.\n", id);
    };
};

int Phantom::counter = 0;

Phantom getPhantom()
{
    Phantom autoPhantom;

    return autoPhantom; // THIS CAN'T BE SAFE
}

int main()
{
    Phantom phantom;

    phantom = getPhantom();

    phantom.speak();

    return 0;
}

I get this output:

Phantom 1 constructed.
Phantom 2 constructed.
Phantom 2 destructed.
Phantom 2 destructed.
Phantom 2 speaks.

It's the fourth line in the output that confuses me.

Phantom 1 is constructed automatically when main is entered.

Phantom 2 is constructed automatically when getPhantom is entered.

Phantom 2 is destructed automatically when getPhantom is exited (which is why I believe returning it from getPhantom is unsafe).

But after that I'm confused. According to the debugger, getPhantom has returned before the fourth line of output appears. When Phantom's destructor is called the second time, the call stack is this:

main
~Phantom

In a managed language, I could see how this line:

phantom = getPhantom();

would destroy Phantom 1, but it wouldn't touch Phantom 2. And this is C++, not Java.

What causes the second call to Phantom 2's destructor?

Stevens Miller
  • 1,387
  • 8
  • 25
  • 7
    Anytime you want to count constructor/destructor calls you need to remember to also print out copy constructor calls. – NathanOliver Jul 20 '16 at 12:40
  • @NathanOliver Ahhh, so do you mean that the object constructed automatically in `getPhantom` is copied, not returned directly? That would explain how `return ` could be safe. Is that it? – Stevens Miller Jul 20 '16 at 12:43
  • 3
    **Of course** returning objects by value is safe. The language would be fundamentally broken otherwise. – Jonathan Wakely Jul 20 '16 at 12:45
  • 2
    There should really be a FAQ about how to count constructors and destructors properly, this question comes up constantly. – Jonathan Wakely Jul 20 '16 at 12:46
  • 3
    [Rule of three](https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)). Obey! – n. m. could be an AI Jul 20 '16 at 12:47
  • Other than the obvious, if you had _initialised_ `main()::phantom` from the result of `getPhantom()`, rather than assigning, RVO would probably have gotten rid of the copy/destruction. Not that it's a problem ("CAN'T BE SAFE") either way. – underscore_d Jul 20 '16 at 12:47
  • 3
    @StevensMiller Yes. There are things like copy elision and return value optimization but returning something means you copy that something into the function return space. – NathanOliver Jul 20 '16 at 12:47
  • 1
    Is that your full output? – Benjamin Lindley Jul 20 '16 at 12:50
  • @NathanOliver Thanks for the amplification. You and the others who answered my actual question made it all perfectly clear. I did look for stuff on this before posting, but somehow missed the "constant" asking of my question. Taking copy constructors (something I was ignorant of) into account makes it all clear. Much appreciated! – Stevens Miller Jul 20 '16 at 12:52
  • Possible duplicate of [Why is the destructor of the class called twice?](http://stackoverflow.com/questions/2627540/why-is-the-destructor-of-the-class-called-twice) - Note that was the first first hit in google when I typed "destructor called twice" – Martin Bonner supports Monica Jul 20 '16 at 12:53
  • @MartinBonner There is a subtle difference though in the way the copies are made. The answer to print the copies is right onbut the reason the copy was made is different. We really need to get a canonical on this. – NathanOliver Jul 20 '16 at 12:57
  • @MartinBonner I Googled "automatic destructor call," and something similar here. I should have Googled "destructor called twice." I should have known that. I'm sorry. – Stevens Miller Jul 20 '16 at 13:03

6 Answers6

8

You return a copy. Therefore the variable in getPhantom() is destroyed at the end of the scope and you are left with its copy that has also id 2. It is because on return it calls copy constructor (also default one) that does not increment the id.

Resurrection
  • 3,916
  • 2
  • 34
  • 56
  • Sam Varshavchik's answer was also very helpful, as were all the comments that directed me towards copy constructors. Those of you like Nathan and the others who answer the question without insulting the questioner, are great colleagues. Thanks! – Stevens Miller Jul 20 '16 at 13:01
5

You are forgetting to properly account for:

  1. Copy constructors.

  2. Assignment operators.

In both of these cases you will wind up with more than one object having the same id, with both objects ending up printing the same id in their destructor. In the case of a copy constructor no message gets printed in the constructor, since you do not define your own copy constructor. In the case of an assignment operator, the id assigned in the constructor get overwritten by a duplicate id from another object. This is what happens here:

phantom = getPhantom();

As such, your accounting gets this wrong.

Sam Varshavchik
  • 114,536
  • 5
  • 94
  • 148
3

I will comment on your concern that it's not safe to return an object with automatic storage:

Phantom getPhantom()
{
    Phantom autoPhantom;

    return autoPhantom; // THIS CAN'T BE SAFE
}

If that wouldn't be safe, then C++ would be pretty useless, don't you think? To see what I am talking about, just replace the type with... say int:

int getPhantom()
{
    int autoPhantom = 0;

    return autoPhantom; // How does this look to you now?
}

To be clear: it's perfectly safe, because you are returning the value (i.e. a copy of the object).

What is unsafe is to return a pointer or a reference to such an object:

int* getInt()
{
   int a = 0;
   return &a;
}
bolov
  • 72,283
  • 15
  • 145
  • 224
  • Yup, you're quite right, I can see that now. I come from a long, long background of C programming, and am new to C++. I confused returning an object with returning a pointer to a local. Thanks for helping me clear my thinking on that. – Stevens Miller Jul 20 '16 at 12:53
  • 1
    @StevensMiller C was no different in this regard whatsoever. – underscore_d Jul 20 '16 at 12:55
  • ??? C allows returning `struct`s by value (it isn't done enough in my view, but it is valid). – Martin Bonner supports Monica Jul 20 '16 at 12:55
  • That's true, but it is a common mistake to return a pointer to a local, and, somehow, that's just what this looked like to me. – Stevens Miller Jul 20 '16 at 13:00
  • @StevensMiller, as you wrote in the question, "this is C++, not Java." Returning an object means return by value, not return by reference. – Jonathan Wakely Jul 20 '16 at 13:05
  • @Martin Bonner Maybe I was using incomplete implementations, but I think I recall back in the early days of C that one could not return a struct, but one could return a pointer to a struct. The "trick" was to malloc that struct, not return a pointer to an automatic. Perhaps I'm wrong, but that's what I seem to remember from my PDP-11 days. – Stevens Miller Jul 20 '16 at 13:27
  • 1
    Ah, right. Pre-standardization C is a very different story. But C89 was standardized nearly 30 years ago now. – Martin Bonner supports Monica Jul 20 '16 at 13:32
  • Yes, well, I did say, "long, long," didn't I :) ? Old habits die hard, and my bosses used to (back in the early '80s) care a *lot* about things like writing code that would compile on as many platforms as possible, saving CPU cycles, and so on. When I learned my C, that's just how we did it. Never too old to learn a new trick, though (or an old one ;). ) – Stevens Miller Jul 20 '16 at 13:36
2

Instead of questioning whether such simple code results in destroying an object that was never constructed, or destroying something twice, consider that it's far more likely the object was constructed and each object is only destroyed once, but you didn't track the constructions and destructions accurately.

Now think about other ways objects can be constructed in C++, and consider what happens if a copy constructor is used at any point. Then consider how you return a local object from a function, and whether the copy constructor gets used.

If you want to improve your test code print out the value of the this pointer in the destructor, and you'll see that your attempt to give each object an ID is flawed. You have multiple objects with different identities (i.e. addresses in memory) but the same "ID".

Jonathan Wakely
  • 166,810
  • 27
  • 341
  • 521
  • I didn't assume that, Jonathan. I said the output confused me. – Stevens Miller Jul 20 '16 at 12:47
  • 4
    "// THIS CAN'T BE SAFE" and "which is why I believe returning it from getPhantom is unsafe" is a pretty big assumption. – Jonathan Wakely Jul 20 '16 at 12:50
  • Golly, you got me there, Jonathan. I'm such a fool. – Stevens Miller Jul 20 '16 at 12:55
  • @JonathanWakely: *"Before you start assuming the compiler doesn't understand C++"* -- You seem to be implying that a C++ compiler will always tell you if you are doing something unsafe. – Benjamin Lindley Jul 20 '16 at 12:57
  • @BenjaminLindley, that would be quite a huge generalization from what I said. Returning an object from a function is hardly a dark and dangerous corner of the language. – Jonathan Wakely Jul 20 '16 at 13:04
  • @JonathanWakely: No more of a generalization than the one you made in equating the statement "THIS CAN'T BE SAFE" to the assumption that "the compiler doesn't understand C++". – Benjamin Lindley Jul 20 '16 at 13:06
  • @BenjaminLindley the question title is "Why is automatic object's destructor called twice?" which is what I was referring to, but I'll edit the answer. – Jonathan Wakely Jul 20 '16 at 13:09
  • Edited. But I still think assuming that a destructor running a second time after the local has been destroyed could be a valid interpretation of the test output (rather than a flaw in OP's logic) pretty clearly suggests an assumption that something unsafe and illogical is normal in C++. – Jonathan Wakely Jul 20 '16 at 13:17
2

Phantom autoPhantom;

return autoPhantom; // THIS CAN'T BE SAFE

It's perfectly safe. The function is returning the object by value, that is, a copy will be made and returned (possibly elided by "return value optimization" (RVO)).

If the function had returned a reference or pointer to the local variable, then you would be right and it would be unsafe.

The reason for the "extra" destructor call is simply that the local variable is destroyed and later the copy that was returned is destroyed.

Jesper Juhl
  • 30,449
  • 3
  • 47
  • 70
  • "If the function had returned a reference or pointer to the local variable, then you would be right and it would be unsafe." He could do that safely if he bound it to const reference I think. – Resurrection Jul 20 '16 at 12:49
  • True, a *const* reference would keep the referenced temporary alive until the reference variable goes out of scope. But that's tricky and error prone. – Jesper Juhl Jul 20 '16 at 12:51
  • 3
    Returning a const reference to a local variable does *not* extend the lifetime of it. – TartanLlama Jul 20 '16 at 12:53
  • Whoops, yup, the lifetime would only be extended if the return was an rvalue... right? Anyway, using `const &` to extend lifetime only makes sense for function arguments IMO, not returns. If you want people to get an object whose lifetime is extended by their receipt thereof, then it should be returned by value. But then I'm sure there's probably some handy use for this that would disprove me, if I could only think of it ;-) – underscore_d Jul 20 '16 at 12:55
  • 1
    @underscore_d No, lifetimes of automatic variables cannot be extended by returning a reference to them, lvalue or rvalue. – TartanLlama Jul 20 '16 at 12:57
  • 1
    You're right, only if he returned a temporary could you extend the lifetime. Got confused. Tricky and error prone... – Jesper Juhl Jul 20 '16 at 12:59
  • @TartanLlama That's imprecise wording on my part. I really meant if it was an rvalue due to being an unnamed temporary - not an lvalue that's `std::move()`d, thus becoming an rvalue. Thanks for the correction. – underscore_d Jul 20 '16 at 12:59
  • 1
    No, returned temporaries cannot have their lifetimes extended via returning by reference either. `[class.temporary]/5.2: The lifetime of a temporary bound to the returned value in a function return statement is not extended; the temporary is destroyed at the end of the full-expression in the return statement.` – TartanLlama Jul 20 '16 at 13:03
  • 1
    I think you guys are getting confused between returned temporaries and temporaries bound like this `const Foo& f = make_foo();`, which *is* valid. – TartanLlama Jul 20 '16 at 13:05
1

Add such code to your class:

Phantom& operator=(const Phantom& inPhantom)
{
    printf("Assigning.\n");
}

and you'll see that 2nd object is not destroyed twice. The explnanation is simplier. On assignment operation first object changes all it's fields values to the values of the second object, but it is not destroyed. And it is still object number one. Your updated example: http://cpp.sh/6b4lo

katagaeshi
  • 43
  • 1
  • 5
  • That is immensely illuminating. Thanks so much for taking the time to edit my code as you did. Makes it really clear that my "id" wasn't telling me what i thought it was. – Stevens Miller Jul 20 '16 at 13:17