-3

I have made a minial example for my question:

#include <bits/stdc++.h>
using namespace std;
class Data
{
};
class Base
{
private:
    Data data1_;
    int x{1};

public:
    explicit Base(Data data) : data1_(data)
    {
        cout << "Base constructor" << endl;
    }
    Base(Base &&b) : data1_(std::move(b.data1_)), x(std::move(b.x))
    {
        cout << "Base move constructor" << endl;
    }
    virtual ~Base()
    {
        cout << "Base destructor" << endl;
    }
};
class Derived : public Base
{
private:
    Data data2_;

public:
    explicit Derived(Data data1, Data data2) : Base(data1), data2_(data2)
    {
        cout << "Derived constructor" << endl;
    };
    Derived(Base &&b, Data data2) : Base(std::move(b)), data2_(data2)
    {
        cout << "Derived move constructor" << endl;
    }
    ~Derived()
    {
        cout << "Derived destructor" << endl;
    }
};
void foo()
{
    unique_ptr<Base> owner = make_unique<Base>(Data());

    unique_ptr<Base> *ptr1 = &owner;

    Base *base = ptr1->release();

    ptr1->reset(new Derived(std::move(*base), Data()));

    cout << sizeof(Base) << endl;
}
int main()
{
    foo();
    return 0;
}

In the above example, sometimes, there will be 16 bytes memory leak in my code.(detected by clang LeakSanitizer):

foo()function is what i need to do, i.e. replace Base with Derived depending on the pointer to unique_ptr and the move constructor of Derived.

Under this scenario, what's my best solution to avoid memory leak and make full use of the move constructors? What's more, what's the best practice to deal with non-class members in move constructor?

njtech_hemengjie
  • 1
  • 1
  • "Howerver, if I change the move constructor of Base to the following case" what following case? Please post a [mcve] of the code that does have the error. – 463035818_is_not_an_ai Apr 26 '23 at 08:21
  • 3
    `b.~Base();` you should not be calling the destructor manually here – 463035818_is_not_an_ai Apr 26 '23 at 08:22
  • 2
    in this particular case the "best" would be the rule of 0. The compiler generated move, copy and destructor are all fine here – 463035818_is_not_an_ai Apr 26 '23 at 08:23
  • you should read here https://en.cppreference.com/w/cpp/language/rule_of_three. scroll down till you find the Rule of zero – 463035818_is_not_an_ai Apr 26 '23 at 08:25
  • @463035818_is_not_a_number, thx, I have changed the above example. – njtech_hemengjie Apr 26 '23 at 08:35
  • 3
    Nothing deletes your original Base. You call `release` on the smart pointer which tells the smart pointer to give _you_ the ownership of it. You then neither delete that pointer nor hand it over to another smart pointer. You therefore leak it. – Mike Vine Apr 26 '23 at 08:43
  • What exactly is the point of `ptr1`? It's only confusing, and you're doing the same as `owner.release()` and `owner.reset(...)`. – molbdnilo Apr 26 '23 at 08:47
  • @MikeVine `ptr1->reset(new Derived(std::move(*base), Data()));` This line doesn't move the original Base to the Derived? – njtech_hemengjie Apr 26 '23 at 08:48
  • 1
    @njtech_hemengjie `move` doesn't actually move anything; it's just a cast to rvalue reference. – molbdnilo Apr 26 '23 at 08:50
  • @molbdnilo In my real code, what I have is the pointer to the unique_ptr, I need to upgrade the original Base to the Derived via this pointer. – njtech_hemengjie Apr 26 '23 at 08:51
  • you have: `unique_ptr *b = ...;`. To replace this just do `*b = make_unique(std::move(**b), ...);` or whatever you need. Or even better convert to a reference first so you can be clearer: `unique_ptr *b = ...; auto& bRef = *b; bRef = make_unique(std::move(*bRef), ...);` – Mike Vine Apr 26 '23 at 09:00
  • You're overthinking this. `ptr1->reset(new Derived(std::move(**ptr1), Data()));` or `*ptr1 = make_unique(std::move(**ptr1), Data());` is all you need. – molbdnilo Apr 26 '23 at 09:04
  • 1
    Side notes: (1) [#include ](https://stackoverflow.com/questions/31816095/why-should-i-not-include-bits-stdc-h) (2) [using namespace std](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice) – wohlstad Apr 26 '23 at 09:04
  • @molbdnilo what is the difference between `**ptr1` and `*base`，I think they have same type, why `**ptr1` works? – njtech_hemengjie Apr 26 '23 at 09:18
  • 1
    It's not the **ptr which works, it's the fact that with this new code you DONT tell the smart pointer that you'll take care of the lifetime via calling `release`. IF you call `release` you are responsible for calling `delete`. The new code doesn't call `release` and so works fine. – Mike Vine Apr 26 '23 at 09:26
  • @njtech_hemengjie It works because you let the `unique_ptr` do its job and manage the lifetime of the objects for you. `**ptr1` is the same as `*owner`. (In my experience, it helps to not think of the "smart pointers" as *pointers* at all, because they aren't.) – molbdnilo Apr 26 '23 at 09:28
  • To put it another way: `**ptr1` is the object managed by `owner`; `*base` is the object that *used to be* managed by `owner`, but that you have assumed responsibility for by asking `owner` to release it. – molbdnilo Apr 26 '23 at 09:40

3 Answers3

3

Here:

unique_ptr<Base> owner = make_unique<Base>(Data());
unique_ptr<Base> *ptr1 = &owner;
Base *base = ptr1->release();
ptr1->reset(new Derived(std::move(*base), Data()));
cout << sizeof(Base) << endl;

you seem to assume that moving from base will call its destructor, but it does not. release takes ownership away from the unique_ptr. You now have a raw owning pointer, that you must either delete or hand to someone to manage the objects life time for you. You do not do that. Hence the leak.

Moving is orthogonal to lifetime. After you moved from *base the object is still alive. If you release the ownership form a unique pointer you must take care to delete the object:

unique_ptr<Base> owner = make_unique<Base>(Data());
unique_ptr<Base> *ptr1 = &owner;
Base *base = ptr1->release();
ptr1->reset(new Derived(std::move(*base), Data()));
delete base;                                          // <---
cout << sizeof(Base) << endl;

... or just don't call release. There are certainly easier ways to do what you want, its just not completely clear what you want to do. The code presented doesn't do anything useful other than demonstrating a leak.

Concerning the question of how to properly implement the move constructor in your class, you should read here about the rule of 0: https://en.cppreference.com/w/cpp/language/rule_of_three. In your particular example, there is no reason for custom implementation of any of the special members.

Mike Vine
  • 9,468
  • 25
  • 44
463035818_is_not_an_ai
  • 109,796
  • 11
  • 89
  • 185
  • So, in the `ptr1->reset(new Derived(std::move(*base), Data()));`, `ptr1` will only manage `Derived` instead of `base` for me? – njtech_hemengjie Apr 26 '23 at 09:05
  • @njtech_hemengjie Sorry, I dont understand why you expect this line to take ownership of `base`. `reset` does not magically see through the parameter you pass to see that `*base` is reference to an object whose lifetime is not managed by anybody – 463035818_is_not_an_ai Apr 26 '23 at 09:07
  • Take the comments'advice, `ptr1->reset(new Derived(std::move(**ptr1), Data()));` can solve my question.However `**ptr1` and `*base` are all `Base`, what made `**ptr1` worked? – njtech_hemengjie Apr 26 '23 at 09:21
  • @njtech_hemengjie Sorry, I dont know what you mean with "what made `ptr` worked?". `ptr` in your code is just unecessary confusion, its just a pointer to `owner`. Whatever you do to `ptr` you could as well do to `owner` directly. – 463035818_is_not_an_ai Apr 26 '23 at 09:25
0

Let me take a summary.

In the presented example, memory leak happened because I don't take responsibily to delete it.(The caller of release is responsible for deleting the object.)

If I want to upgrade Base to Derived through a pointer to unique_ptr<Base>, I should write as follows:

unique_ptr<Base> *b = ...;
*b = make_unique<Derived>(std::move(**b), Data());

or

unique_ptr<Base> *b = ...;
b->reset(new Derived(std::move(**b), Data()));

In this way, I choose continue let unique_ptr<Base> help me manage the old Base, and hand the new Derived to it, ask it manage these objects for me.

In the presented example, I misunderstand that new Derived will help me delete the original Base.

If what I say is wrong, please comment.

njtech_hemengjie
  • 1
  • 1
0

After release, it means that unique_ptr won't manage pointer, so it become raw pointer, you should delete it.

unique_ptr::release: Releases the ownership of the managed object, if any. get() returns nullptr after the call. The caller is responsible for deleting the object.

Adrian Mole
  • 49,934
  • 160
  • 51
  • 83
lemt
  • 1
  • 1