Why is c++ prematurely destroying my string variable?

Question

This error is one of the most mysterious I've gotten in probably years. I've been trying things back and forth for hours now and can't understand it.

In the code below, I am trying to dynamically decide which derived class to instantiate. I use the function parameter particle_type in order to determine that.

But since

Particle p;

is not allowed, since you have to initialize references, I am trying to do it with a pointer as below.

Particle* p;
if (particle_type == "Featherr") { //This is never called because of the extra "r" I put, so ignore this block
        p = &FeatherParticle(particle_name, position, rotation, velocity, gravityEffect, lifeLength, scale);
}
else {
    p = &Particle(particle_name, position, rotation, velocity, gravityEffect, lifeLength, scale);
}

Particle particle1 = *p;
Particle particle2 = Particle(particle_name, position, rotation, velocity, gravityEffect, lifeLength, scale);

particles.push_back(particle1);
particles.push_back(particle2);

Now the particle2 is just me debugging, but it highlights this bug* very well. I put a breakpoint at particles.push_back(particle1); and found that the particle object stored by particle1 was broken:

I do not understand why the string is set to "" here. I can only guess it has to do with the scoping, because if I set the string outside the else statement, it works. Which makes my jaw drop at how stupid this functionality is *which is why I think it may be a bug or near-bug functionality.

`pointer = &Object()` might work in Rust or Go, but it does not work in C++! Have you enabled all available compiler warnings? — jtbandes, May 07 '20 at 00:29
Yes. Namely, the object pointed to by p is destroyed instantly, even before exiting the `else {}` block. The object pointed to by particle2 is also destroyed instantly and not valid. — Michaël Roy, May 07 '20 at 00:33
Why are you messing with pointers? Why not just `particles.push_back(makeParticle(particle_type, particle_name, position, rotation, velocity, gravityEffect, lifeLength, scale));` — Mooing Duck, May 07 '20 at 00:34
The use of the constructor would be fine within the `push_back` parenthesis, a copy will be inserted in the array. — Michaël Roy, May 07 '20 at 00:36
I guess you are using MSVC which allows taking address of rvalues for some reason (instantly creating a dangling pointer) — M.M, May 07 '20 at 00:44
I was going to say compile with `/permissive-` to disallow this, but it baffles me that this still compiles in that mode. Looks like you still need `/Za` to disable this extension. There are APIs out there that take things by pointer for performance where this is safe, but it's not a particularly common thing and hindsight is 20/20 on this being the default behaviour, but they're kind of stuck with it. — chris, May 07 '20 at 00:46
An anternative cannot be suggested without seeing how `particles` is defined — M.M, May 07 '20 at 00:47
@MooingDuck I wanted to be able to choose whichever Particle to create, whether it be FeatherParticle or regular Particle or whichever. But your suggestion does make sense, it just isn't quite as elegant to read I think. — TheForgot3n1, May 07 '20 at 18:41

Remy Lebeau · Accepted Answer · 2020-05-07T00:42:08.653

You are setting p to point at temporary objects that are destroyed immediately after the assignment, thus leaving p dangling. You need to use new instead:

Particle* p;
if (particle_type == "Featherr") { //This is never called because of the extra "r" I put, so ignore this block
    p = new FeatherParticle(particle_name, position, rotation, velocity, gravityEffect, lifeLength, scale);
}
else {
    p = new Particle(particle_name, position, rotation, velocity, gravityEffect, lifeLength, scale);
}
...

But then, you would be slicing the object when assigning *p to particle1, so use a reference instead (or simply get rid of particle1 since you don't need it):

Particle &particle1 = *p;

But then, you are trying to push Particle objects into particles, not Particle* pointers. So you would slice particle1 anyway. Polymorphism only works when using pointers/references, so you should be storing Particle* pointers (which means using new for particle2 as well):

std::vector<Particle*> particles;

...

Particle *p;
...
Particle *particle2 = new Particle(particle_name, position, rotation, velocity, gravityEffect, lifeLength, scale);

particles.push_back(p);
particles.push_back(particle2);

And don't forget to delete anything you new when you are done (and make sure Particle has a virtual destructor):

for(size_t i = 0; i < particles.size(); ++i) {
    delete particles[i];
}

Or, in C++11 and later:

for(auto *p : particles) {
    delete p;
}

But, it would be better to use std::unique_ptr to handle that delete for you automatically. Use std::make_unique() (C++14 and later), don't use new directly at all if you can avoid it:

std::vector<std::unique_ptr<Particle>> particles;

...

std::unique_ptr<Particle> p;
if (particle_type == "Featherr") { //This is never called because of the extra "r" I put, so ignore this block
    p = std::make_unique<FeatherParticle>(particle_name, position, rotation, velocity, gravityEffect, lifeLength, scale);
    // or:
    // p.reset(new FeatherParticle(particle_name, position, rotation, velocity, gravityEffect, lifeLength, scale));
}
else {
    p = std::make_unique<Particle>(particle_name, position, rotation, velocity, gravityEffect, lifeLength, scale);
    // or:
    // p.reset(new Particle(particle_name, position, rotation, velocity, gravityEffect, lifeLength, scale));
}

auto particle2 = std::make_unique<Particle>(particle_name, position, rotation, velocity, gravityEffect, lifeLength, scale);
// or:
// std::unique_ptr<Particle> particle2(new Particle(particle_name, position, rotation, velocity, gravityEffect, lifeLength, scale));

particles.push_back(std::move(p));
particles.push_back(std::move(particle2));

Can you elaborate a little what you mean by "You are setting p to point at temporary objects that are destroyed immediately after the assignment"? What is the temporary object in this situation, and why is it temporary (I didn't want it to be immediately destroyed) — TheForgot3n1, May 07 '20 at 18:49
In `p = &FeatherParticle(...);`, `FeatherParticle(...)` creates a *temporary* unnamed (anonymous) `FeatherParticle` object, then `&` returns the memory address of that object which is then assigned to `p`, then the object goes out of scope on `;` and is destroyed, leaving `p` dangling. Same with `p = &Particle(...);` with a *temporary* `Particle` object. — Remy Lebeau, May 07 '20 at 18:57
But if I store its address in a pointer, shouldn't C++ understand that I want to keep my reference to it? — TheForgot3n1, May 07 '20 at 18:59
No, that is not how C++ works. The temporary objects are being created in *automatic* memory storage, so they are destroyed when they go out of scope. For what you are attempting, you need to create the objects in *dynamic* memory storage instead — Remy Lebeau, May 07 '20 at 18:59
But if I do like @MooingDuck suggests: `particles.push_back(makeParticle(particle_type, particle_name, position, rotation, velocity, gravityEffect, lifeLength, scale));` why does that work? It is also creating a temporary object, right? — TheForgot3n1, May 07 '20 at 19:02
The only way that could work with polymorphic types is if `makeParticle()` creates the returned object *dynamically* via `new` (or `std::make_unique()`) and then returns a pointer (or `std::unique_ptr`) to the object. — Remy Lebeau, May 07 '20 at 19:02
Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/213363/discussion-between-remy-lebeau-and-theforgot3n1). — Remy Lebeau, May 07 '20 at 19:06

Why is c++ prematurely destroying my string variable?

1 Answers1