C++ Avoid Temporary Object Destruction using Copy Constructor

Question

I have a std::vector of Trees, a custom type, and I have a loop which pushes back a tot of Trees to the vector. The problem is: I need push back temporary Tree objects, but, at the same time, I have to avoid them getting deallocated (I thought adding a rvalue reference constructor to the Tree class would have solved the problem, but it didn't).

Tree class:

class Tree
{
    public:
        Tree();
        Tree(Coords, Pixel, Pixel, uint8_t);
        Tree(const Tree&);
        Tree(const Tree&&);
        ~Tree();

        Tree& operator=(const Tree&);
        Tree& operator=(const Tree&&);

        void draw();
        void chop(uint8_t);

        Coords getCoords() const {return log->coords;}

        const Log& getLog() const
        { 
            return *log;
        }

        const Crown& getCrown() const
        {
            return *crown;
        }

    private:
        Log*     log;
        Crown* crown;
};

Tree::Tree():
    log(nullptr), crown(nullptr)
{

}

Tree::Tree(Coords c, Pixel wd, Pixel lvs, uint8_t h):
    log(new Log(c, wd, h)), crown(new Crown({c.x, static_cast<coordsCounter>(c.y + h)}, lvs, getRandBetweenEqu(3, 4)))
{
    draw();
}

Tree::Tree(const Tree& tree):
    log(new Log(tree.log->coords, tree.log->texture, tree.log->height)),
    crown(new Crown(tree.crown->coords, tree.crown->texture, tree.crown->width))
{
    draw();
}

Tree::Tree(const Tree&& tree):
    log(new Log(tree.log->coords, tree.log->texture, tree.log->height)),
    crown(new Crown(tree.crown->coords, tree.crown->texture, tree.crown->width))
{
    draw();
}

Tree& Tree::operator=(const Tree& tree)
{
    if(log != nullptr) delete log;
    if(crown != nullptr) delete crown;

    log   = new Log(*tree.log);
    crown = new Crown(*tree.crown);

    return *this;
}

Tree& Tree::operator=(const Tree&& tree)
{
    if(log != nullptr) delete log;
    if(crown != nullptr) delete crown;

    log   = new Log(*tree.log);
    crown = new Crown(*tree.crown);

    return *this;
}

void Tree::draw()
{
    log->draw();
    crown->draw();
}

Tree::~Tree()
{
    chop(0);
}

void Tree::chop(uint8_t h)
{
    if(crown != nullptr)
    {
        delete crown;
        crown = nullptr;
    }

    if(h == 0)
    {
        if(log != nullptr)
        {
            delete log;
            log = nullptr;
        }
    } else
    {
        if(h <= log->height)
        {
            log->chop(h);
        }
    }
}

Function pushing back trees:

void growTrees(uint8_t m, uint8_t M)
{
    Coords treeCoords;

    for(coordsCounter i = 5; i < grid.getMaxXY().x - 4; i += getRandBetweenEqu(m, M))
    {
        if((treeCoords = getFirstBlock({i, static_cast<coordsCounter>(grid.getMaxXY().y - 1)}, 's', grassTexture) + Coords{0, 1}) != Coords{0, 1})
        {
            trees.push_back(Tree(treeCoords, woodTexture, leavesTexture, getRandBetweenEqu(minTreeHeight, maxTreeHeight)));
        }
    }
}

If you construct them, you need to deconstruct them, even if moved... — Jarod42, Apr 12 '21 at 16:08
Which is the real problem? Do your class follows [rule of 3/5/0](https://en.cppreference.com/w/cpp/language/rule_of_three)? — Jarod42, Apr 12 '21 at 16:09
*but, at the same time, I have to avoid them getting deallocated* -- Sounds like your `Trees` class is lacking proper copy semantics. — PaulMcKenzie, Apr 12 '21 at 16:10
Responses may be better if you can include code for the `Trees` class. In modern C++ you can often get the best results by not defining constructors ilor assignment operators. You might also emplace rather than pushing onto the vector. — ariels, Apr 12 '21 at 16:17
why do you need to avoid the temporary objects getting deallocated? — user253751, Apr 12 '21 at 16:27
Because when `Tree`s get deallocated they also get cancelled from the `grid`, which is kinda like the level of the game (they disappear from the screen, basically) — DarkoNaito_09, Apr 12 '21 at 16:28
@DarkoNaito_09 *Because when Trees get deallocated they also get cancelled from the grid, which is kinda like the level of the game* -- IMO, that's too much "business logic" to be placed in a copy constructor. A copy constructor's job should be simple -- make a copy, without all of the side-effects going on. Maybe you should consider making the class non-copyable. — PaulMcKenzie, Apr 12 '21 at 17:01
I get it, but I really dislike the idea of having draw() and chop() functions separated from the constructor and distructor. Also, making the class non-copyable wouldn't solve the problem I'm having at the moment. — DarkoNaito_09, Apr 12 '21 at 17:05
@DarkoNaito_09 -- If it is decided that an object is copyable, it must make logical sense as to what it means to make a copy. For example, stream classes in C++ are not copyable. What would it mean to make a copy of, say, a file stream? Two files being written or read from? That's why streams are not copyable, because it doesn't make sense. However, the stream classes are *movable*, and maybe you should just drop the copying and make the class just moveable. — PaulMcKenzie, Apr 12 '21 at 17:26
Also, note that copying is something the *compiler* can do, without you actually doing anything explicit. So now you will have copies going off all over the running of the program, and you have little to no control over it. That's why copying must be simple, efficient, and with no (visible) side effects. — PaulMcKenzie, Apr 12 '21 at 17:29
I =deleted all the constructors/functions which involved copying, and now I'm getting `stl_algobase.h:400:18: error: use of deleted function 'constexpr Tree& Tree::operator=(const Tree&)'` — DarkoNaito_09, Apr 12 '21 at 20:35
Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/231046/discussion-between-darkonaito-09-and-paulmckenzie). — DarkoNaito_09, Apr 12 '21 at 20:43

score 0 · Answer 1 · edited Apr 12 '21 at 19:47

0

Your move constructor/assignment operator are actually copying the objects, they should take non-const rvalue references and set the moved from object to some empty value.

Tree::Tree(Tree&& tree):
    log(std::exchange(tree.log, nullptr)),
    crown(std::exchange(tree.crown, nullptr))
{
    draw();
}
Tree& Tree::operator=(Tree&& tree)
{
    std::swap(tree.log, log);
    std::swap(tree.crown, crown);
    return *this;
}

edited Apr 12 '21 at 19:47

Remy Lebeau

555,201
31
458
770

answered Apr 12 '21 at 16:44

Alan Birtles

32,622
4
31
60

In fact, you can combine the copy-assignment and move-assignment operators together into a single implementation: `Tree& Tree::operator=(Tree tree) { std::swap(tree.log, log); std::swap(tree.crown, crown); return *this; }` This allows the caller to decide whether to copy-construct or move-construct the `tree` parameter as needed. – Remy Lebeau Apr 12 '21 at 19:50

C++ Avoid Temporary Object Destruction using Copy Constructor

1 Answers1