3
\$\begingroup\$

This question is an improvement of this one here: Implementing a unique_ptr - PPP Stroustrup exercise

Here's my new code, following the suggestions by @JDługosz.

  • I agree with the fact that T* operator is much better than T *operator, but the code formatter of VSCode does this automatically, I'll try to fix it. Sorry for that.

  • I implemented the move semantics, and testing it seems it's okay. I'd like to have a check on my implementation as well. In particular, if in the move constructor I write std::move(p.ptr) only, then I have a pointer being freed was not allocated error, and I don't understand why since after the move, the object from which I "stole" the resource should become a nullptr, but apparently it's not.

If I write in the definition of that move constructor function p.ptr=nullptr (as I did) it works. Why is that?

#include <iostream>
#include <vector>
#include <cassert>

template <typename T>
class Unique_Ptr
{
private:
    T *ptr;

public:
    explicit Unique_Ptr(T *p = nullptr) noexcept : ptr{p} {}

    //Copy semantics is deleted
    Unique_Ptr(const Unique_Ptr &) = delete;
    Unique_Ptr &operator=(const Unique_Ptr &) = delete;

    //Move semantics
    Unique_Ptr(Unique_Ptr &&p) noexcept : ptr{std::move(p.ptr)}
    {
        p.ptr = nullptr;
    }

    Unique_Ptr &operator=(Unique_Ptr &&p) noexcept
    {
        ptr = std::move(p.ptr);
        p.ptr = nullptr;
        return *this;
    }

    ~Unique_Ptr()
    {
        delete ptr;
    };

    T *operator->() const noexcept { return ptr; }

    T operator*() const { return *ptr; }

    T *get() const noexcept { return ptr; }

    T *release() noexcept
    {
        auto tmp = ptr;
        ptr = nullptr;
        return tmp;
    }
};

int main()
{
    // Constructor test
    Unique_Ptr p{new int{2}};
    assert(*p == 2);

    Unique_Ptr<int> p_default;
    assert(p_default.operator->() == nullptr);

    //Test with pointer to a vector
    Unique_Ptr pp{new std::vector<int>{1, 2, 3}};
    assert(pp->size() == 3);

    //release() test
    auto test_release = pp.release();
    assert(pp.get() == nullptr);
    assert(test_release->size() == 3);

    //Move cstr
    Unique_Ptr<int> ptr1(new int{2});
    Unique_Ptr<int> ptr2 = std::move(ptr1);
    assert(*ptr2 == 2);
    assert(ptr1.get() == nullptr);

    //Move assignment
    Unique_Ptr<int> ptr3{};
    ptr3 = std::move(ptr2);
    assert(*ptr3 == 2);
    assert(ptr2.get() == nullptr);

    return 0;
}

EDIT:

I tried to write the following move assignemtn, following @Zeta's suggestion:

Unique_Ptr &operator=(Unique_Ptr &&p) noexcept
{
    this->swap(p);
    return *this;
}

where swap is

void swap(Unique_Ptr &p)
{
    std::swap(ptr, p.ptr);
}

Now things are working properly and I'm not leaking memory with the move assignment. However, I'd like my move assignment to act like a std::unique_ptr, i.e. I'd like that the object from which I "move" is nullptr. How can I achieve that?

Following the suggestion by @JDługosz, I would say:

Unique_Ptr &operator=(Unique_Ptr &&p) noexcept
{
    this->swap(p);
    Unique_Ptr<T> tmp{p.release()}; 
    return *this;
}

so now tmp will be released after the exit from the move assignment, and p will be a nullptr after the release

\$\endgroup\$
9
  • 5
    \$\begingroup\$ Is the code cut off? It seems incomplete. \$\endgroup\$
    – indi
    Sep 13 '21 at 19:53
  • 1
    \$\begingroup\$ Move assignment is bugged. Imagine by some complicated implications in code algorithm you get something equivalent to p = move(p); that will result in memory leak with your unique_ptr and p being empty when it wasn't originally - not the desirable outcome. And ptr = move(p.ptr); is meaningless - move performs copy on trivial types. \$\endgroup\$
    – ALX23z
    Sep 13 '21 at 20:33
  • \$\begingroup\$ Also your replacement for default constructor is explicit- it isn't good. You cannot write something like x= {} for clearing x. \$\endgroup\$
    – ALX23z
    Sep 13 '21 at 20:38
  • \$\begingroup\$ @all I have updated my code with the main. \$\endgroup\$ Sep 14 '21 at 7:52
  • \$\begingroup\$ The main cannot compile without a template argument for the first p and pp. \$\endgroup\$
    – Zeta
    Sep 14 '21 at 9:22
1
\$\begingroup\$

Your move assignment leaks memory:

Unique_Ptr<int> foo();

...

Unique_Ptr<int> p{new int{2}};
p = foo();                      // <- leaks the original int

I recommend you to add a swap(Unique_Ptr & other) and use that in your move assignment operator. That way, the Unique_Ptr&&'s destructor will take care of freeing the original value.

\$\endgroup\$
4
  • \$\begingroup\$ I added an edit to my question where I followed your suggestion. As I wrote, my main concern now is to have my Unique_Ptr to act like a std::unique_ptr: I'd like the moved object to be a nullptr, not to just swap the values! \$\endgroup\$ Sep 14 '21 at 10:41
  • 1
    \$\begingroup\$ @bobinthebox easy: swap, then release p. \$\endgroup\$
    – JDługosz
    Sep 14 '21 at 14:22
  • \$\begingroup\$ @JDługosz I've edited my question for the last time. I added a last snippet with my interpretation of your last comment. Is that what you meant? \$\endgroup\$ Sep 14 '21 at 14:29
  • \$\begingroup\$ @bobinthebox see my Answer. You're beating around the answer and not seeing the simplicity of what you are doing. \$\endgroup\$
    – JDługosz
    Sep 14 '21 at 14:39
1
\$\begingroup\$

There’s no point to doing a design review, since you’re reimplementing something—there’s no real designing being done. So I’ll just go straight to the code review.

Code review

It’s good that you wrote some tests to confirm your unique pointer is working. However, because you’ve jammed the actual code and the tests together, you’ve basically ruined the code. It is no longer an independent type that you can reuse. It’s now spaghetti sauced with the test code.

What you should do is put your code in its own header (or, in C++20 or better, a module), and then only have your test code include that header (or import the module). That way, once your tests all pass and you know your code is good, you can just go ahead and use it as-is in other code by including/importing… no need to edit—and thus potentially break—working code.

#include <iostream>
#include <vector>
#include <cassert>

This is one of the problems I’m referring to. None of these headers are required for Unique_Ptr… they’re all only used for the tests. But because you’ve mixed everything together, a reviewer has to read the entire program in order to realize that.

    T *ptr;

You mentioned that you realize this is poor C++ style—the proper way to write this in C++ would be T* ptr;—but this is what VSCode is inflicting on you. Fair enough.

    explicit Unique_Ptr(T *p = nullptr) noexcept : ptr{p} {}

Ugh, no. I know some people rave about defaulted parameters, but I am not a fan. Default arguments cause way more problems than they’re worth.

For example, you may not realize it, but you no longer have a constructor that takes zero arguments. Oh, you can still construct the type with zero arguments… but that’s not the same thing. The difference is subtle, but real. Under the hood, the compiler is transforming every auto p = Unique_Ptr<int>{} into auto p = Unique_Ptr<int>{static_cast<int*>(nullptr)}… which means every single default construction has a hidden extra, useless parameter passed, which you’re paying for every single time (will it be optimized away? maaaaybe, depending on the context).

There are plenty of other problems, too, which can show up when you try to do advanced stuff like function pointers. But because this constructor is also explicit… well, this is the problem with that:

auto func1() -> Unique_Ptr<int>
{
    return {};  // won't compile, because your "default constructor" is
                // explicit
}

auto func2() -> std::unique_ptr<int>
{
    return {};  // WILL compile, because std::unique_ptr's default constructor
                // is not explicit
}

My advice? Just don’t do default arguments. Just don’t. They’re acceptable as a quick-and-dirty hack, sometimes, but for real library types… don’t.

Just write out the two constructors:

    Unique_Ptr() noexcept : ptr{nullptr} {}

    explicit Unique_Ptr(T* p) noexcept : ptr{p} {}

    // possible constructor #3?
    //
    // when called with nullptr literal (like: Unique_Ptr<int>{nullptr}),
    // eliminates the need to actually pass the null pointer value as a
    // parameter... possibly giving a very tiny performance gain
    explicit Unique_Ptr(std::nullptr_t) noexcept : ptr{nullptr} {}

Even better, use a member initializer:

template <typename T>
class Unique_Ptr
{
private:
    T* ptr = nullptr;

public:
    constexpr Unique_Ptr() noexcept = default; // defaulted! (also, i added constexpr)

    explicit constexpr Unique_Ptr(T* p) noexcept : ptr{p} {} // meh, no help here

    explicit constexpr Unique_Ptr(std::nullptr_t) noexcept {} // no need to set ptr manually

    constexpr Unique_Ptr(Unique_Ptr&& p) noexcept // no need to set ptr manually
    {
        swap(*this, p);
    }

    // ...

If you really feel tempted to use default arguments for a constructor particularly… don’t. Delegate instead.

    //Move semantics
    Unique_Ptr(Unique_Ptr &&p) noexcept : ptr{std::move(p.ptr)}
    {
        p.ptr = nullptr;
    }

    Unique_Ptr &operator=(Unique_Ptr &&p) noexcept
    {
        ptr = std::move(p.ptr);
        p.ptr = nullptr;
        return *this;
    }

Okay, there’s some confusion about move semantics going on here.

What do you think happens when you do:

auto some_int = 5;

auto p = &some_int;
auto q = std::move(p);

// what is q now?
// what is p now?

The answer to the first question is easy. p used to be the address of some_int, and then you moved p into q. That means q is now the address of some_int, so *q is 5. No problem.

But… what is p?

There is only one correct answer: 🤷🏼.

That’s what moving is all about. When you move x into y, y now has the value that was previously in x, and x has… “?”. That’s the secret power of moving; that’s what the optimization is all about. One way to think of moving is as an optimized copy that leaves junk in the copied-from object… if you’re not using the copied-from object again, having junk in it is no problem.

  • Copying means taking the value of xduplicating it… and then putting it in y. You’ve gone from having one copy of the data to two… and that can be expensive.
  • Moving means taking the value of x… putting it in y… and then leaving “junk” in x. You don’t have to create a whole new copy of the data. Instead you can pull garbage from thin air—which can be done cheaply—and leave that in x.

Okay, technically the rule is that you have to leave x in an “unspecified but valid” state. But there are lots of ways to very cheaply make an unspecified but valid state for most things. For example, it’s very cheap to make an empty string. But you should generally not assume anything about the moved-from object. In fact, a moved-from string is not guaranteed to be an empty string; it could be anything. Generally, you can only do two things with a moved-from object:

  1. Destroy it.
  2. Reassign it.

That is why moving is… USUALLY… so much more efficient than copying. You don’t need to create a duplicate of existing data so both objects have the same data… you can make cheap garbage data to leave in the moved-from object.

Where is all this leading? We’re almost at the payout… just one more thing to consider.

For a lot of types… particularly built-in types like int and pointers… the cheapest “junk” you can store in it is… whatever happened to be in there already. Remember, moving can be thought of as an optimized copy, where you don’t care about the copied-from object (because it’s about to be destroyed or reassigned). But for built-in types… copying is already as cheap as it can be. So optimizing the copy of a built-in type is… just copying. Which means moving a built-in type is just copying.

Which means:

auto some_int = 5;

auto p = &some_int;
auto q = std::move(p);

// what is q now?
// what is p now?

The answer is that p is the same thing it was before the move! The move is just a copy. There’s no way to make it more efficient.

Now let’s consider your move constructor, slightly rewritten just for clarity:

Unique_Ptr(Unique_Ptr&& p)
{
    ptr = std::move(p.ptr);

    // at this point, ptr has whatever was in p.ptr

    // but what is in p.ptr?
    //
    //  *   the *theoretical* answer is: *shrug*, an unspecified pointer value
    //  *   the *realistic* answer is: the same value it had before; moving
    //      didn't change it
}

Either way, you have a problem.

With the realistic answer, it means that you now have two “unique” pointers that think they own whatever ptr is pointing to: *this and p. When both are destroyed, boom, you have a double-free. Crash, or at least UB. (The message you’re getting—“pointer being freed was not allocated”—is probably because the first object is being destroyed and properly freeing the memory… then the second object gets destroyed and tries to free the memory again, but now that memory has already been deallocated… so “the pointer being freed wasn’t allocated”, or more correctly, the pointer being freed is not allocated now… it was, but is no longer.)

With the theoretical answer, it means that p.ptr could be…. ANYTHING. Which, I’m sure you can imagine, is hella bad. Because when p gets destroyed, it’s going to try to free whatever it thinks p.ptr is pointing to in some random area of memory. That’s almost certainly going to cause chaos.

So either way, realistic or theoretical, you’re screwed.

That is why you need the p.ptr = nullptr; line:

Unique_Ptr(Unique_Ptr&& p)
{
    ptr = std::move(p.ptr);

    // at this point, ptr has whatever was in p.ptr

    // p.ptr has:
    //  *   (theoretical): anything!!!
    //  *   (realistic): the same value as this->ptr

    // so:
    p.ptr = nullptr;

    // now ptr has what used to be in p.ptr, and p.ptr is nullptr, so when
    // p is destroyed, it won’t do anything (because it is an empty unique
    // pointer). safe!
}

Now, in theory, there is no difference between between:

    ptr = std::move(p.ptr);
    p.ptr = nullptr;

    // or, without move

    ptr = p.ptr;
    p.ptr = nullptr;

except that the first way is theoretically faster. (Because, remember, move is just optimized copy, which you can use when you are going to be getting rid of the source anyway, as you are here by reassigning it immediately.) Will it be realistically faster? No. In reality, copying a pointer is already as optimized as it’s going to get, so there will be no difference between the two code blocks.

So it makes no practical difference whether you move or copy here… copying a raw pointer is already as fast as can be, so moving just degenerates to copying. But, theoretically, the right thing to do is move, because you’re reassigning the source immediately anyway.

I hope that clears up a few things:

  1. Moving is, from one point of view, just an optimization of copy that you can use in cases where you don’t care about preserving the value of the source object (because you’re about to destroy or reassign it anyway).
  2. Generally, you should think of the value of a moved-from object as “🤷🏼”. Do not assume a moved from object is the same as an “empty” or “default-constructed” object. That is true only for some types (std::unique_ptr happens to be one, raw pointers are not).
  3. Because the value of a moved-from object is unspecified, it either has be to be destroyed right away, or reassigned… never make any other assumptions about it.

And because you can’t assume anything about a moved-from value, that’s why you have to put a “safe” value in p.ptr… which is nullptr.

Okay, one more thing to cover: the relationship between moving and swapping.

Suppose you have a type foo whose values are very expensive to construct and destroy. Suppose you have two foo objects a, and b, which have the values bar and qux respectively. So the initial state is:

  • a ⇒ “bar”
  • b ⇒ “qux”

What happens after a copy assignment a = b?

After that, you should have:

  • a ⇒ “qux”
  • b ⇒ “qux”

But the key thing is how you got there. In order to get to that state, you had to:

  1. Destroy the value of “bar” in a.
  2. Construct a new value of “qux” in a.

Of course, construction and destruction are expensive, so that makes copying expensive.

What happens after a move assignment a = std::move(b)?

After that, you should have:

  • a ⇒ “qux”
  • b ⇒ ???

Where the value of b is valid, but unspecified; it could be anything.

Here’s the million dollar question: where should the value in b come from? what should it be?

We could, in theory, just make up an arbitrary (valid) value, like “zzz” and use that, to get:

  • a ⇒ “qux”
  • b ⇒ “zzz”

This is perfectly legal. The problem is… it’s hella wasteful. To get this, we have to:

  1. Destroy the value of “bar” in a.
  2. Move the value of “qux” from b to a.
  3. Construct a new value of “zzz” in b.

That’s even less efficient than copying!

We could also just copy… that would give a perfectly legitimate result, too. But can we do better?

In fact we can. We need a value to put in b. We don’t care what that value is. Well, we have a perfectly valid value that was in a. Why throw that away? Why not just put that in b? In other words:

  • a ⇒ “qux”
  • b ⇒ “bar”

Now we no longer have to construct or destroy any values. We just move the value from a into b, and the value from b into a.

And that’s just swapping.

That’s why standard practice is to implement an efficient swap, and then write the move operations using swap. Doing that spares us from having to worry about constructing and destructing new values when doing moves.

This is the standard form for (almost!) ALL classes you will ever write:

class foo
{
public:
    // move ops
    constexpr foo(foo&& other) noexcept
    {
        // set up to a sensible default state (possibly by delegating to a
        // default constructor)

        swap(*this, other);
    }

    constexpr auto operator=(foo&& other) noexcept -> foo&
    {
        swap(*this, other);
        return *this;
    }

    // hidden friend swap function
    friend constexpr auto swap(foo& a, foo& b) noexcept -> void
    {
        // swap data members efficiently here
    }

    // ... rest of class
};

This is the same pattern you should use for Unique_Ptr, almost exactly. It’s just The Way To Do It™.

(Note: @JDługosz suggests that move-assignment should be swap() then reset(). That’s not wrong. Remember, the moved-from value should be “valid, but unspecified”. Leaving other with the value that was originally in *this is valid… leaving other with nothing after a reset() is also valid. So… you can go either way. The reason doing the reset() is unnecessary is because since other is going to be moved-from, it’s going to be either reassigned or destroyed in a moment anyway. In other words, even if you don’t reset immediately… it’s going to happen in a moment regardless. So I wouldn’t bother with the reset()… but you could do it if you felt like. There are pros and cons to either option; it really comes down to an engineering decision. But, broadly speaking, either way is fine.)

    T *operator->() const noexcept { return ptr; }

    T operator*() const { return *ptr; }

You have noticed that operator-> is noexcept, while operator* is not. That’s how std::unique_ptr does it (IIRC). But do you know why? There’s an important lesson there.

Finally, there are quite a few interesting operations std::unique_ptr has that you’re missing. You can’t compare Unique_Ptrs. You can’t use them like this: if (p) { ... }. You should also look into making it constexpr. (If you graduate from C++14 to C++20, you can make it completely constexpr.)

I would also suggest taking the time to learn a proper test framework, rather than trying to hand-roll your own tests. Proper testing is one of the most important skills you can learn as a programmer… not just as a C++ programmer, but as a programmer in general.

And since you’re stuck with the details of move ops, I would suggest doing something a little inelegant and sticking TONS of trace statements in your class, so you can observe how all the values of all the variables change with each operation. Tracing what happens to this->ptr and p.ptr before and after ptr = std::move(p.ptr) will help give you insight into what’s going on. It’s how I learned all the nitty-gritty details of this kind of stuff. You could also use a debugger, if you know how to (but, honestly, I never use a debugger, so when I do have to, I find them clunky; trace statements—just simple std::cout statements—work well enough).

\$\endgroup\$
2
  • \$\begingroup\$ thanks a lot for your answer. It's full of details and I had to read it carefully in order to grasp everything. Just one last thing: operator-> is marked as noexcept because it works even if ptr is nullptr. On the other hand, operator * may throw an exception if ptr is nullptr. In that case, a nullptr will be de-referenced, which is something not legal. This is my motivation: did I say it correctly, or am I missing something? \$\endgroup\$ Sep 15 '21 at 22:51
  • \$\begingroup\$ Yup, that’s pretty much it! I recommend not focusing on exceptions, but rather thinking more generally; think of noexcept as meaning “this function cannot fail”. Functions like get() and release() cannot fail; assuming the Unique_Ptr is in a valid state, those functions will never fail to do what they promise. But a function like std::make_unique<T>() could fail; it promises it will return a std::unique_ptr<T> that points to a new T, but if allocating or constructing might T fail, and if that happens, the function can’t keep its promise… thus, it can’t be noexcept. \$\endgroup\$
    – indi
    Sep 17 '21 at 19:34
0
\$\begingroup\$

Couple of things I would add:

You can not explicitly construct with a nullptr:

 Unique_Ptr<int>  pnull{nullptr};

So I would add a constructor that accepts an explicit nullptr:

 Unique_Ptr(std::nullptr_t)
      :ptr(nullptr)
 {}

It would be nice to allow for ineritance and allow up pointers to be moved down.

 class Base {};
 class D1: public Base {};

 Unique_Ptr<D1>   derived{new D1};

 Unique_Ptr<Base> base = std::move(derived); // Would be nice to support this.

It should look like this:

 template<typename U>
 Unique_Ptr(Unique_Ptr<U>&& src) noexcept
 {
      Unique_Ptr<T>  tmp(src.release());
      // Note: Assume you add a swap.
      swap(tmp);
 }
 template<typename U>
 Unique_Ptr& operator=(Unique_Ptr<U>&& src) noexcept
 {
      Unique_Ptr<T>  tmp(src.release());
      swap(tmp);
      return *this;
 }

I would add is a bool conversation operator.

Like normal pointers, its nice to be able to easily and simply test a pointer is not null just by using it in a test.

Unique_Ptr<int>   x = getData();

if (x) {
    // Do stuff with x
}

So I would add this:

explicit operator bool() const {return ptr;}

The other thing I would do is put your pointer into its own namespace rather than leaving it in the global namespace.


Obviously, Zeta already pointed out the bug with a leak of a pointer on move assignment.

Personally, I would write the assignment operator like this:

void swap(Unique_Ptr &p) noexcept // Notice the noexpcet
{
    swap(ptr, p.ptr);
}
Unique_Ptr &operator=(Unique_Ptr &&p) noexcept
{
    Unique_Ptr<T> tmp{release()}; 
    swap(p);
    return *this;
}

// Add a friend swap
friend void swap(Unique_Ptr& lhs, Unique_Ptr& rhs)
{
    lhs.swap(rhs);
}
\$\endgroup\$
4
  • \$\begingroup\$ Thanks @MartinYork for your useful comments! Before starting to add those features, I'd like to have a correct implementation of my move assignment. Is the last snippet I wrote in my edit okay, in your opinion? \$\endgroup\$ Sep 14 '21 at 18:39
  • \$\begingroup\$ @bobinthebox Done. I wrote three articles on smart pointers. lokiastari.com/series \$\endgroup\$ Sep 14 '21 at 18:49
  • \$\begingroup\$ It seems, at its core, really identical to mine, except the fact that I wrote Unique_Ptr<T> tmp{p.release()}; , while you wrote it without the p. Is it the same? How is it possible it's compiling? \$\endgroup\$ Sep 14 '21 at 18:55
  • \$\begingroup\$ Okay, now I see. You're applying the release to this. You implementation is equivalent to Unique_Ptr<int> tmp {this->release()}; std::swap(ptr,p.ptr); return *this \$\endgroup\$ Sep 14 '21 at 19:03
0
\$\begingroup\$

and I don't understand why since after the move, the object from which I "stole" the resource should become a nullptr, but apparently it's not.

Because you didn't implement that. You can't count on the full semantics of unique_ptr when you are writing said unique_ptr.

The only thing std::move does is cast the value category. It does not do anything to change the value. Likewise, the assignment of a plain pointer does not affect the right-hand-side!

Primitive (built-in) pointers don't have any special move semantics. So, std::move has no effect when you write

Unique_Ptr(Unique_Ptr &&p) noexcept : ptr{std::move(p.ptr)} {}

it is no different than just writing

Unique_Ptr(Unique_Ptr &&p) noexcept : ptr{p.ptr} {}

The copy construction of the ptr member, a built-in pointer, doesn't care whether the right-hand-side is an lvalue or rvalue. It just copies the bits, like all the built-in primitive machine types.

The magic of move semantics transferring ownership is what this class is implementing. So you program the move constructor to transfer the value rather than just copy it.


For the move assignment operator:

Unique_Ptr& operator= (Unique_Ptr&& p) noexcept
{
    std::swap (ptr, p.ptr);
    p.reset();
}
\$\endgroup\$
3
  • \$\begingroup\$ I'm sorry, but if I use your last snippet (adding return *this at the end) it turns out that I'm leaking memory. Also, my p.release() returns a pointer, how can that line be valid? @JDługosz \$\endgroup\$ Sep 14 '21 at 15:14
  • \$\begingroup\$ I think you meant p.reset()? Is that correct? \$\endgroup\$ Sep 14 '21 at 15:40
  • \$\begingroup\$ @bobinthebox right \$\endgroup\$
    – JDługosz
    Sep 14 '21 at 20:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.