Move-aware noexcept stack in C++ I'm learning C++, and move semantics and exception safety seem to be a top priority when using the language. I implemented the simplest data structure I could think of.

#ifndef STACK_CPP11_HPP
#define STACK_CPP11_HPP

#include &lt;vector&gt;

namespace stack {

template &lt;typename T&gt;
class Stack {
private:
  std::vector&lt;T&gt; storage;

public:
  Stack() : storage() {}

  Stack(const Stack&lt;T&gt; &amp;other) {
    *this = other;
  }

  Stack(Stack&lt;T&gt; &amp;&amp;other) {
    *this = other;
  }

  ~Stack() {
  }

  Stack&lt;T&gt; &amp;operator=(const Stack&lt;T&gt; &amp;rhs) {
    if (this != &amp;rhs)
      storage = rhs.storage;

    return *this;
  }

  Stack&lt;T&gt; &amp;operator=(Stack&lt;T&gt; &amp;&amp;rhs) {
    if (this != &amp;rhs)
      storage = rhs.storage;

    return *this;
  }

  bool top(T &amp;ret) const noexcept {
    if (storage.empty())
      return false;

    ret = storage.back();
    return true;
  }

  void push(T item) noexcept {
    storage.push_back(std::move(item));
  }

  bool pop() noexcept {
    if (storage.empty())
      return false;

    storage.pop_back();
    return true;
  }
};

} // namespace stack

#endif // STACK_CPP11_HPP


I tested the code, and it uses the move constructor wherever possible.
Here are the tests:

#include &lt;cassert&gt;
#include &lt;string&gt;

#include "stack.hpp"

using namespace stack;

struct Box {
  std::string label;

  Box(std::string l) : label(l) {}

  friend void swap(Box &amp;lhs, Box &amp;rhs) {
    using std::swap;
    swap(lhs.label, rhs.label);
  }

  Box(const Box &amp;other) {
    label = other.label;
  }

  Box(Box &amp;&amp;other) {
    swap(*this, other);
  }

  ~Box() {
  }

  Box &amp;operator=(Box rhs) {
    swap(*this, rhs);
    return *this;
  }
};

int main(void) {
  Stack&lt;int&gt; stack1;

  int ret;
  assert(!stack1.top(ret));

  stack1.push(100);
  assert(stack1.top(ret));
  assert(stack1.pop());
  assert(ret == 100);

  auto stack2 = Stack&lt;Box&gt;();

  Box box("bar");
  assert(!stack2.top(box));

  stack2.push(std::move(Box{"foo"}));
  assert(stack2.top(box));
  assert(stack2.pop());
  assert(box.label == "foo");

  return 0;
}


Is there a way to improve the code with regards to move semantics and having an overall noexcept behavior? Also, since I'm fairly new to C++, is there anything I could improve in general?
Move Semantics

One of the rules you have to remember is:
A named variable will not bind to an r-value ref parameter.

void doMoreStuff(Type const&amp; x) {std::cerr &lt;&lt; "Normal Reference\n";}
void doMoreStuff(Type&amp;&amp; x)      {std::cerr &lt;&lt; "RValue Reference\n";}
void doStuff(Type&amp;&amp; x)
{
    doMoreStuff(x); // Here you are using a named variable
}                   // So this will not bind to an r-value ref.
                    // So it will call the top version above.
                    // The `&amp;&amp;` just mark the bind point they
                    // don't change the type of the variable in
                    // any special way.


So your Move Constructor does not do what you think it is doing.

  Stack(Stack&lt;T&gt; &amp;&amp;other) {
    *this = other;  // You are using a named variable.
  }                 // So this will bind to the "Copy Assignment"


To make it work the way you think, you need to use std::move:

  Stack(Stack&lt;T&gt; &amp;&amp;other) {
    *this = std::move(other);
  }


Copy And Swap Idiom

You are implementing the Constructors in terms of Assignment. I suppose it's not wrong. But it is very non-standard. Usually it is done the other way around - implement Assignment in terms of the Constructor (it even has a name: The Copy and Swap Idiom).

  Type&amp; operator=(Type const&amp; rhs)
  {
       Type copy(rhs);    // Make a copy of the rhs.
       swap(copy);        // Swap the content of this and copy
       return *this;
  }


This can even be simplified. You can remove the explicit copy and use an implicit copy by taking the parameter by value:

  Type&amp; operator=(Type copy)
  {
       swap(copy);        // Swap the content of this and copy
       return *this;
  }


Move by swap

The simplest way to implement a move is to use swap. You swap the content of the current object with the source object. The great thing about this is move semantics are supposed by exception safe as is swap. This also has the advantage of delayed destruction (the destruction of the object is delayed until the source object is destroyed; this gives an opportunity for the resources to be re-used).

So two things wrong with your move are that you forgot to mark them noexcept and, as mentioned above, you don't actually move because you use the named parameter.

Type(Type&amp;&amp; rhs) noexcept
   : member(default init)
   , ....
{
    swap(rhs);
}
Type&amp; operator=(Type&amp;&amp; rhs) noexcept
{
    swap(ths);
    return *this;
}
void swap(Type&amp; other) noexcept)
{
    using std::swap;
    swap(member,  other.member);
    ...
}
friend void swap(Type&amp; lhs, Type&amp; rhs) {
    lhs.swap(rhs);
}


Rule of Zero

If all the members already obey the rule of three/five and you don't have any special resource management to do in your class, then the rule of zero springs into effect.

The rule of zero means you don't need to define any constructors/assignment/destructors as the default compiler-generated versions will work perfectly for you.

Since your only data member is storage and this is a std::vector, then you are covered because std::vector obeys the rule of three and does all the correct resource management for you. So you don't need to specify any of the copy or move constructors as the compiler will correctly generate them for you.

Premature Optimization (is actually a pessimization)

This test

if (this != &amp;rhs)
  storage = rhs.storage;


actually decreases performance in normal circumstances (I can't claim it always makes things worse but it usually does). You see, the problem is that self assignment is extremely rare (your code needs to work for it, but in reality it hardly happens).

This means you are adding a branch into your code (one of the worst things to do for optimizations) that is basically never taken. So you have to execute this test even though it is never taken. So you make the normal code path longer and it has a branch.

Now it does pay off if you do self assignment. But if you multiply the cost of checking the branch by the number of times per actual self assignment then subtract the cost of an extra copy then on average you are taking longer (thus a pessimization).

The normal way to implement assignment using the copy and swap always does a copy even though this is hideously more expensive on self assignment than it needs to be and it still pays off for normal usage of strings and vectors.

Pushing Efficiently

You sort of half do it.

void push(T item) noexcept {           // You make a copy of the parameter.
    storage.push_back(std::move(item));// Then you move the copy.
}


But what happens if I want to move an item into the stack? It could be more efficient than making a copy of it first. Also if you are going to copy delay the copy until the end.

// A moving push
void push(T&amp;&amp; item) noexcept {
    storage.push_back(std::move(item));
}
// A copy push
void push(T const&amp; item) noexcept {
    storage.push_back(item);
}


The third choice we have is to build an item in place. Some large types may be easier to create in-place on the stack rather than pushing or moving them (say the constructor takes a single integer but creates a huge object that is expensive to create or move).

template&lt;typename... P&gt;  // template var arg (can be many parameters.
void push(P&amp;&amp;... args) {
    storage.emplace_back(std::forward&lt;P&gt;(args)...);
}


Top efficiency

When you call top() you always make a copy of the top item. But what about when you want to mutate the top item. Do you have to call item = top();pop();/*modify item*/;push(item) to mutate the top item?

Also it means you have to construct an object externally (a empty object) before you can copy into it. Not all types can have an empty object. Not all types are cheap to construct. Additionally copying the item from the stack may be expensive. So this is not a desirable interface. 

I would suggest you simply return a reference to the top item. That way it can be mutated in place or if the user wants they can make a copy by assigning to a local variable.

This does mean that your top() can not return true/false on successes but I would suggest that should be its own function to test for empty anyway.

bool empty() const   {return storage.empty();}
T&amp;       top()       {return storage.back();}
T const&amp; top() const {return storage.back();}


Notice I don't even check to see if there is an item. This is because the calling code can check with a call to empty(). The reason is I don't want to check if there is no need to check and if there is a need to check then the user can call empty(). If you need a checked version then you can add a specific checked version.

This is exactly why most C++ containers have checked and unchecked version of the interface. You should not need to force a check (as that will be inefficient).

while(!stack.empty())
{
    std::cout &lt;&lt; stack.top() &lt;&lt; ",";   // No need to copy if I am printing.
    stack.pop();  // Should not force a check in pop.
                  // We already know from context there is an item in
                  // the stack.
}


Your original code would have looked like this:

T  tmp;                        // You have to build a tmp object.
                               // Is this expensive?
while(stack.top(tmp))          // Check here.
{
    std::cout &lt;&lt; tmp &lt;&lt; ", ";
    stack.pop();               // Another check here. Even though
                               // we know the stack is not empty at
                               // this point.
}


Some minor notes:

Usually user types and namespace begin with an upper case letter, while objects and functions begin with a lower case letter.

This makes user types easier to spot.

 Stack::Stack&lt;Int&gt;  aStack;


Also the type modifiers * and &amp; are part of the type. So in C++ (unlike C) they are usually grouped with the type not the variable.

Stack(const Stack&lt;T&gt; &amp;other)
// More normally written like:
Stack(const Stack&lt;T&gt;&amp; other)
// Or like this:
Stack(Stack&lt;T&gt; const&amp; other)


The standard comment

Please avoid using namespace X;

using namespace stack;


This is really bad idea and will get you into a lot of trouble in larger code bases. So it is best not to get into the habit of this. Prefer to always prefix your types with the namespace.

stack::Stack&lt;Int&gt;   aStack;


If you have an exceedingly long (or nested namespace) you can use a namespace alias to simplify it (I usually use short three letter aliases).

namespace TSt = ThorsAnvil::Stack;

TSt::Stack&lt;Int&gt;     aStack;


See Why is “using namespace std” considered bad practice?.
Best Answer

Using Rule of zero to rewrite:

namespace stack {

// Exception safe moveable stack.
// No constructors or detructors are needed as the members
// do all the work correctly.
template &lt;typename T&gt;
class Stack {
private:
  std::vector&lt;T&gt; storage;

public:

  bool empty()   const noexcept {return storage.empty();}
  T const&amp; top() const noexcept { // still no exception looking at object is safe
      return storage.back();      // will not cause an exception.
  }                               // It may be undefined if you top an empty stack.
  T&amp;       top()       noexcept {
      return storage.back();
  }

  void push(T const&amp; item) {     // Adding an item is not exception safe.
      storage.push_back(item);   // As adding an item to a vector may
  }                              // cause reallocation.
  void push(T&amp;&amp; item) {
      storage.push_back(std::move(item));
  }

  void pop() noexcept {
      storage.pop_back();
  }
};