Design review
So, basically, you would like to remake std::stack. That’s cool, it’s not a bad learning project at all. It is actually possible to make a much better version of std::stack, for a number of reasons (mostly because std::stack is so old).
The std::stack interface could be improved
You have gone with a very minimal stack interface, which is fine. It’s pretty close to what std::stack has, too. But do keep in mind that this is a very restrictive interface. For example, you have made it possible to put non-copyable, move-only types into the stack… but there is no way to get them back out. You can view them (with top()). And you can remove them (with pop()). But you can’t retrieve them.
See for yourself. We’ll use std::unique_ptr, because that is a standard move-only type:
// First we create a unique pointer.
auto p = std::unique_ptr<std::string>(new std::string{"some text"});
// Now we create the stack:
auto stack = AdapterForStack<std::unique_ptr<std::string>>{};
// Now we put the pointer in the stack (this technically won't work for other
// reasons I'll get to shortly, but it *should* work):
stack.push(std::move(p));
// We can *view* the pointer:
std::cout << *(stack.top()) << '\n';
// But there is no possible way to *retrieve* the pointer:
auto q = stack.top(); // won't work, non-copyable
auto q = std::move(stack.top()); // won't work, can't move from const
With std::stack, it is possible to retrieve the pointer because std::stack has two overloads of top()… one that returns const&, and one that just returns &. If you get the non-const reference, you can move from that. So it “works”, but leaves the top element of the stack in an indeterminate, moved-from state… which is not good.
If std::stack were designed today, it might have a different interface. Instead of pop() returning void, it would probably pop the top object and return it. This just makes sense:
// the obvious way to push something onto the stack
stack.push(something);
// the obvious way to pop something off the stack
auto something = stack.pop();
(It is also how the proposed concurrent queue would work.)
Anywho, if all you’re aiming to do is replicate the std::stack interface, that’s fine. But do keep in mind that the std::stack interface is not great. It is as ancient as the STL (which predates the standard library). It was good back then… not so much now.
Which end of the stack?
Your stack works by appending to elements to the front of the stack. That is… peculiar. Most stacks work by appending to the end of the stack. This includes std::stack.
And, in fact, you have a bug. You didn’t fully test your stack. Or rather, your testing code is broken.
To illustrate the bug, let’s first fix the broken testing code. In test_mulit_lower_container(), you take the container type as a template parameter Container. However… look at how you actually construct the stack:
AdapterForStack<T, std::list<T>> stack{};
You see? You don’t use Container. You have hard-coded std::list<T>.
So first we have to fix that to:
AdapterForStack<T, Container> stack{};
Now your testing code is actually testing different container types. You can compile and run it to make sure everything is working.
To illustrate the bug, just change the push line from:
stack.push(default_val);
to:
stack.push(std::move(default_val));
If you try to compile the code now, it will fail.
What went wrong? Well, when we were using stack.push(default_val), we were calling the lvalue overload of push():
void push( const value_type& value )
{
m_continer_ptr->insert(m_continer_ptr->begin(), value);
return;
}
But when we switched to stack.push(std::move(default_val)), we are now calling the rvalue overload of push():
void push(value_type&& value )
{
m_continer_ptr->push_front(std::move(value));
return;
}
See the difference? The lvalue overload calls .insert(). The rvalue overload calls push_front().
The problem is… std::vector does not have .push_front(). Thus, compile error.
This is (part of) why std::stack puts elements at the end of the container. That way it can use .push_back() and .pop_back(), which are supported by std::vector, std::deque, and std::list.
If you are going to push elements to the front, that’s okay (although, it will be very slow for std::vector, and it makes many other operations more complicated). However, you can’t use .push_front() and .pop_front(), because only std::deque and std::list support those… not std::vector.
You could use .insert() and .erase(), as you do.
So, you need to make a decision. Either:
- Change to pushing/popping from the back. That way you can use
.push_back() and .pop_back(), and get maximum performance when using std::vector as the underlying container; … or…
- Don’t use
.push_front(), and only use .insert() and .erase(), and accept that performance with a std::vector will be terrible.
Questions
How to limit T to numeric type and the Container to sequence container?
So that’s two questions. Let’s tackle them one at a time.
First, what do you mean by “numeric type”? I mean, I assume you mean stuff like int, double, and other built-in “math” types. You probably also mean types that do modular arithmetic, like unsigned int. But do you also mean std::complex? Do you also mean third-party “bignum” types?
Let’s start simple. If you want to constrain to integers, there is the concept std::integral. For floating point numbers, there is std::floating_point. So, a first attempt would be:
template <typename T>
concept number = std::integral<T> or std::floating_point<T>;
Except there’s a catch, because std::integral also includes things that are not numbers, like bool, and character types.
So we want something more like this:
template <typename T>
concept character =
std::same_as<T, char>
or std::same_as<T, char8_t>
or std::same_as<T, char16_t>
or std::same_as<T, char32_t>
or std::same_as<T, wchar_t>
;
template <typename T>
concept number =
(
std::integral<T>
and (not std::same_as<T, bool>)
and (not character<T>)
)
or std::floating_point<T>
;
If you want to extend that to support other numeric types like std::complex or “bignum” types, it’s not hard, but I’ll leave that as an exercise for the reader.
With that concept, you’d just constrain your template like this:
template <number T, class Container=std::vector<T>>
class AdapterForStack
Now, for the second part of the question. This is going to be even trickier than the first part. As before, there is no single concept that defines containers. A container is a range… but not all ranges are containers. Nevertheless, a range is a good start:
template <typename T>
concept container = std::ranges::range<T>;
Now, a container is something more than a “pure”, abstract range… especially a container that you can use as a stack. Looking at the container hierarchy, we need at least to be able to read and write (so at least a mutable input iterator), but we need multiple passes (it won’t do if we can only read what we put in the stack once… what would happen if you call top() twice?), so we need at least a mutable forward iterator.
So instead of a plain range, we can require at least a forward range:
template <typename T>
concept container = std::ranges::forward_range<T>;
(As of C++23, you could also test not std::ranges::constant_range<T> to ensure the range is mutable.)
Now, a container, by definition, owns its elements. That’s what differentiates a container from a view. Because it owns its elements, it can’t also be borrowing them. Therefore we can check not std::ranges::borrowed_range<T>. While we’re at it, we could also check that it’s not a view:
template <typename T>
concept container =
std::ranges::forward_range<T>
and not std::ranges::borrowed_range<T>
and not std::ranges::view<T>
;
Finally, if you are going to be requiring that the container supports .insert() and .erase() (or .push_front() or .push_back() or whatever you are going to be using), you can check for that, too:
template <typename T>
concept container =
std::ranges::forward_range<T>
and not std::ranges::borrowed_range<T>
and not std::ranges::view<T>
and requires(
T& container,
std::ranges::iterator_t<T> i,
std::ranges::range_value_t<T> const& lv,
std::ranges::range_value_t<T>&& rv
)
{
container.insert(i, lv);
container.insert(i, rv);
container.erase(i);
}
;
Something like that.
And with that, you’d do:
template <
number T,
container Container = std::vector<T>
>
class AdapterForStack
Note though, while we have pretty much confirmed it’s a container, we never actually checked whether it’s a sequence container. I’m not sure how to go about that off the top of my head. That’s something you can look into on your own.
how to make the type of Container is depend on the type of T?
Now, if you ask this question to a large enough pool of C++ experts, you’ll probably get advice about using template templates (that is not a typo, “template templates” are a thing.) This is bad advice. It will “work” for most containers, but it is a hack.
The correct way to do this constraint is to use something like:
std::same_as<T, std::ranges::range_value_t<Container>>
Which you would use like:
template <
number T,
container Container = std::vector<T>
>
requires std::same_as<T, std::ranges::range_value_t<Container>>
class AdapterForStack
Easy peasy.
About unique pointer
Yes, about unique pointer. I have to admit, I don’t understand what your concern is with your question:
Comparing use an instance of an instance of container(say, make the said instance as an object, but it seems impossible because the stack have many different kinds of ctors, so the lower container should be created dynamically), is it the right choice to use the unique_ptr?
This reads like nonsense to me. “Impossible because the stack may have many different kinds of constructors”? What does that even mean? The stack can have as many constructors as it pleases with no bearing on the on the underlying container:
template<class T, class Container = std::vector<T>>
class AdapterForStack
{
Container _container;
public:
// a default constructor:
AdapterForStack()
// just default construct the underlying container
: _container{}
{}
// an iterator pair constructor:
template <std::input_iterator I, std::sentinel_for<I> S>
AdapterForStack(I first, S last)
// either: if the underlying container supports iterator pair construction:
: _container(first, last)
{}
// or: if it doesn't:
: _container{}
{
std::ranges::copy(first, last, std::back_inserter(_container));
}
// a range constructor:
template <std::ranges::input_range R>
AdapterForStack(R&& rng)
{
std::ranges::copy(std::forward<R>(rng), std::back_inserter(_container));
}
// and just for fun, a constructor from 3 T values:
AdapterForStack(T a, T b, T c)
{
_container.push_back(std::move(a));
_container.push_back(std::move(b));
_container.push_back(std::move(c));
}
// how about a constructor that fills the stack with n copies of v?
AdapterForStack(std::size_t n, T v)
{
std::ranges::fill_n(std::back_inserter(_container), n, v);
}
// and so on...
What is impossible here?
You could even automatically use the underlying container constructors if supported, and fall back on a default if not:
// container can be constructed from iterator pair
template <std::input_iterator I, std::sentinel_for<I> S>
requires std::constructible_from<Container, I, S>
AdapterForStack(I first, S last)
: _container(first, last)
{}
// container can *not* be constructed from iterator pair
template <std::input_iterator I, std::sentinel_for<I> S>
AdapterForStack(I first, S last)
: _container{}
{
std::ranges::copy(first, last, std::back_inserter(_container));
}
There is no need for std::unique_ptr or for any kind of dynamic allocation. Just try it and see.
Code review
#include <vector>
#include <memory>
#include <iostream>
#include <list>
#include <deque>
It is good practice to put includes in a logical order; alphabetical is usually a good idea.
template<class T, class Container=std::vector<T>>
class AdapterForStack
“Adapter for stack” is a bit of a clunky name. “Stack adapter” is nicer, but even just “stack” gets the message across. “Adapter” is the design pattern the class uses, but that information is not really useful for the person using it. All they care about is that it’s a stack.
Put another way, suppose I see auto x = AdapterForStack<int>{};. What do I care about here? What do I need to know to use x? Do I care that this is an adapter? No. Do I care that it is a stack? Yes. That tells me that x is a container that I can probably push and pop to. That’s useful information.
Also, you should always put stuff in a namespace; it is bad form to pollute the global namespace.
template<class T, class Container=std::vector<T>>
class AdapterForStack
{
public:
using value_type = T;
AdapterForStack()
{
m_continer_ptr = std::unique_ptr<Container>(new Container());
}
There is just too much indentation going on here. It makes sense to indent the contents of a class, so you can see what stuff is in the class and what stuff is not. But there is no point in double indenting every method, member, and type. Something more like:
template<class T, class Container=std::vector<T>>
class AdapterForStack
{
public:
using value_type = T;
AdapterForStack()
{
m_continer_ptr = std::unique_ptr<Container>(new Container());
}
wastes a lot less space, and loses no information (because the public: and private: must always be in a class, so there is no need to signal that they are in a class by indenting them).
AdapterForStack()
{
m_continer_ptr = std::unique_ptr<Container>(new Container());
}
The less code you write, and the more you leave to be auto-generated by the compiler, the better.
The only reason you need this default constructor is because you are (unnecessarily) using a pointer to hold a dynamically-allocated instance of the container. If you got rid of the dynamic allocation and just used the container directly as a member, then you don’t need this constructor. You could just write:
AdapterForStack() = default;
You could also add constexpr and conditional noexcept with:
constexpr AdapterForStack() noexcept(std::is_nothrow_default_constructible_v<Container>) = default;
Technically, you could write nothing at all, and get the default constructor automatically. However, you probably want other constructors, in which case you’ll need the above.
Speaking of, you really do need some more constructors. Even if you want to ignore allocators, which is fine, you at least want to be able to construct a stack with some pre-existing content, either using an iterator pair, an initializer list, or a range (or all of the above).
It’s also worth mentioning that with your current design, using a std::unique_ptr, your stack is non-copyable. But there is no reason that need be the case. If you use the container directly as a data member, the stack will be copyable if the underlying container is copyable. (And the same for movability.)
bool empty()
{
return m_continer_ptr->empty();
}
A couple things here.
First, there is no reason this function should ever fail. Either the stack is empty or it is not. There are no other possible logical options. So this function should be marked noexcept.
It should also be marked const, because asking if the stack is empty should never change the stack.
And of course, everything should be constexpr by default.
So:
constexpr auto empty() const noexcept -> bool
{
return m_continer_ptr->empty();
}
However, it’s worth noting that if you are going to use a pointer, you should be more responsible, and check the pointer before dereferencing it. You don’t need to; the only way you could ever get a null pointer is if you move the stack:
auto s1 = AdapterForStack<int>{};
// s1.m_continer_ptr is always valid here
auto s2 = std::move(s1);
// s1.m_continer_ptr is no longer valid
s1.empty(); // NULL POINTER DEREFERENCE!!!
So you could just say it’s an error to do anything with a moved-from stack. That’s how pretty much everything in the standard library works. It would make your stack more dangerous, but more efficient. So you need to weigh the consequences.
This problem goes away if you just don’t use dynamic allocation, and instead put the underlying container directly in the stack.
size_t size()
{
return m_continer_ptr->size();
}
Everything for empty() applies here, too. But also, it should be std::size_t. size_t works for historical reasons… but it will not work in future C++ code. So get in the habit of doing std::size_t (and including one of the required headers).
But there is one more complication: the size_type of the underlying container might not be std::size_t.
The correct thing to do is:
template<
class T,
class Container = std::vector<T>
>
class AdapterForStack
{
public:
using value_type = T; // could also do std::ranges::range_value_t<Container>;
using size_type = std::ranges::range_size_t<Container>;
// ...
constexpr auto size() const noexcept -> size_type
{
return m_continer_ptr->size();
}
OR, just let everything be deduced:
template<
class T,
class Container = std::vector<T>
>
class AdapterForStack
{
public:
using value_type = T;
// ...
constexpr auto size() const noexcept
{
return m_continer_ptr->size();
}
Either way is fine.
void push( const value_type& value )
{
m_continer_ptr->insert(m_continer_ptr->begin(), value);
return;
}
void push(value_type&& value )
{
m_continer_ptr->push_front(std::move(value));
return;
}
You don’t need the return; statements here. They add nothing to the code but clutter.
As mentioned in the design section, you have a bug here. In the rvalue overload of push(), you use push_front()… but std::vector has no push_front(). So you should do this:
constexpr auto push(value_type const& value)
{
m_continer_ptr->insert(m_continer_ptr->begin(), value);
}
constexpr auto push(value_type&& value)
{
m_continer_ptr->insert(m_continer_ptr->begin(), std::move(value));
}
However… do you really want to insert new stack elements to the front of the underlying container?
Consider that there are basically 3 main underlying container types you want to support: std::vector, std::deque, and std::list. Only std::deque and std::list support push_front() and pop_front(). But all three support push_back() and pop_back(). (See the function table for the standard containers.)
There’s more. While you can push to the front of a vector (which you do with m_continer_ptr->insert(m_continer_ptr->begin(), value)), doing so can be incredibly expensive. To push to the front of a vector requires moving every single element in the vector, every time. Pushing to the back of a vector is very fast, though. (Pushing to the front or back of a std::deque or std::list is fast. std::vector is the only one the three where there is a difference.)
There’s still more. Eventually, you’ll probably want to be able to support filling a stack from a sequence, either an iterator pair or a range, with something like: auto stack = AdapterForStack<int>{0, 1, 2, 3, 4, 5, 6, 7, 8};. If you put each element at the front of the underyling container, then you would have to write:
constexpr AdapterForStack(std::initializer_list<T> il)
{
std::ranges::copy(il | std::views::reverse, std::inserter(_container, std::ranges::begin(_container)));
}
// or:
constexpr AdapterForStack(std::initializer_list<T> il)
{
std::ranges::for_each(std::views::all(il), [this] (auto&& v) { this->push(v); });
}
In other words, if you want the last element of the input range to be the first element of the underlying container—which would be the top of the stack—you need to copy the input range into the container backwards. So when the input range is {0, 1, 2, 3, 4, 5, 6, 7, 8}, the container contents will be {8, 7, 6, 5, 4, 3, 2, 1, 0}.
This is… not great. Copying backwards is a lot of work.
If, instead, the top of the stack was at the back of the underlying container, then you could just do a straight-up copy:
constexpr AdapterForStack(std::initializer_list<T> il)
{
std::ranges::copy(std::views::all(il), std::inserter(_container, std::ranges::end(_container)));
}
// or possibly even just:
constexpr AdapterForStack(std::initializer_list<T> il)
: _container(il)
{}
This could be much faster, because it might just be a big-ass block transfer (aka memcpy()).
So there are a lot of very good reasons to have the stack grow to the back of the underlying container, rather than at the front.
std::unique_ptr<Container> m_continer_ptr;
I think you’ve misspelled “container”.