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
x… duplicating 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:
- Destroy it.
- 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:
- 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).
- 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).
- 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:
What happens after a copy assignment a = b?
After that, you should have:
But the key thing is how you got there. In order to get to that state, you had to:
- Destroy the value of “bar” in
a.
- 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:
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:
This is perfectly legal. The problem is… it’s hella wasteful. To get this, we have to:
- Destroy the value of “bar” in
a.
- Move the value of “qux” from
b to a.
- 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:
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).
p = move(p);that will result in memory leak with your unique_ptr andpbeing empty when it wasn't originally - not the desirable outcome. Andptr = move(p.ptr);is meaningless - move performs copy on trivial types. \$\endgroup\$x= {}for clearingx. \$\endgroup\$maincannot compile without a template argument for the firstpandpp. \$\endgroup\$