Can someone please tell me if I'm using unique pointers correctly

Question

I'm trying to use smart pointers more but am not sure if I'm using them correctly. I seem to need to use std::move all over the place. Don't quite understand why, but it's working. What my example code does isn't important (nothing as happens :)), I'm more concerned with the structure of how i'm using these pointers. Thanks for any advice.

class IBehaviour {
public: virtual void Process() = 0;
};

class BehaviourA : public IBehaviour {
    public: virtual void Process() override { /* do behaviour A stuff */ }
};

class BehaviourB : public IBehaviour {
    public: virtual void Process() override {/* do behaviour B stuff */ }
};

class BehaviourC : public IBehaviour {
    public: virtual void Process() override {/* do behaviour C stuff */ }
};

class BehaviourProcessor {
public:
    BehaviourProcessor(int m) {
        if (m == 1)     behaviour = std::make_unique<BehaviourA>();
        else if (m==2)  behaviour = std::make_unique<BehaviourB>();
        else            behaviour = std::make_unique<BehaviourC>();
    }

    std::unique_ptr<IBehaviour> getBehaviour() {
        return std::move(behaviour);
    }

    std::unique_ptr<IBehaviour> behaviour = nullptr;
};

class Container {
public:
    void setProcessor(std::unique_ptr<BehaviourProcessor> ada) {
        adaproc = std::move(ada);
    }

    std::unique_ptr<BehaviourProcessor> getProcessor() {
        return std::move(adaproc);
    }

    std::unique_ptr<BehaviourProcessor> adaproc = nullptr;
};

int main()
{
    Container c;
    std::unique_ptr<BehaviourProcessor> beh = std::make_unique<BehaviourProcessor>(1);
    c.setProcessor(std::move(beh));
    c.getProcessor()->getBehaviour()->Process();

    beh = std::make_unique<BehaviourProcessor>(2);
    c.setProcessor(std::move(beh));
    c.getProcessor()->getBehaviour()->Process();

    beh = std::make_unique<BehaviourProcessor>(3);
    c.setProcessor(std::move(beh));
    c.getProcessor()->getBehaviour()->Process();
}

You probably don't want `getProcessor` to move out the resource. You won't be able to call it every again. You probably want to return a raw non-owning pointer. Currently `c.getProcessor()->getProcessor()->Process();` results in the `IBehaviour` and `BehaviourProcessor` being destroyed, which may be unexpected. — François Andrieux, Apr 28 '22 at 15:00
Fyi, `std::unique_ptr behaviour = nullptr` is redundant. And I concur, you don't want to 'move' out of your getters. You *can* return a *reference* to the members directly to avoid that (whether it's a good idea to do so being debatable itself). — WhozCraig, Apr 28 '22 at 15:02
It also doesn't look like `Container` needs a `std::unique_ptr` at all instead of just having a `BehaviourProcessor` by value. `BehaviourProcessor` doesn't have any polymorphic behavior. — Nathan Pierson, Apr 28 '22 at 15:03
ONLY use the smart pointer where you need to manage the lifetime of the object from. Otherwise, give access to the object it points to, not the smart pointer itself. Smart pointers are just to make sure the object is deleted at the right time, not for generally passing around the system. — Galik, Apr 28 '22 at 15:04

eerorika · Answer 1 · 2022-04-28T15:14:12.003

class IBehaviour {
public: virtual void Process() = 0;
};

std::unique_ptr<IBehaviour> behaviour = nullptr;

behaviour = std::make_unique<BehaviourA>();

This is wrong. By pointing the smart pointer to a base sub object, it will delete through that base pointer, which will result in undefined behaviour because the destructor of the base isn't virtual. Solution: declare ~IBehaviour virtual.

std::unique_ptr<IBehaviour> getBehaviour() {
    return std::move(behaviour);
}

std::unique_ptr<BehaviourProcessor> getProcessor() {
    return std::move(adaproc);
}

These are dubious. A non-rvalue-ref-qualified function should not move from a member. This will probably not be expected by the caller.

It's unclear why you use dynamic allocation for Container::adaproc considering BehaviourProcessor is not a base class. This may be an unnecessary pessimisation. I recommend considering a value member:

class Container {
public:
    BehaviourProcessor adaproc;

Likewise, the getters and setters seem to be largely redundant. The classes could be much simpler as aggregates.

Well I clearly don't understand smart pointers fully :) Thanks everyone for taking your time to get back to me. — user1408542, Apr 28 '22 at 16:03

Can someone please tell me if I'm using unique pointers correctly

1 Answers1