C++ Dynamic array based stack

Overall

Not bad.

Resizing.

The maths is in favor of using a refactoring size of less than 1.63 as it potentially allows for memory to be reused after several allocations.

    static const int m_growth_factor = 2;

But on the other hand all the standard libraries do use 2.

Copy and Swap Idiom

    inline MyStack(const MyStack<T> &rhs) { *this = rhs; }

You are defining the copy constructor in terms of the assignment operator. The idiomatic way to do this is the other way around and define the assignment operator in terms of the copy constructor. Its called the copy and swap idiom. It provides the strong exception guarantee (ie the assignment works or the object is unchanged). Your assignment operator does not provide this.

Pre-Allocation

template <class T>
void MyStack<T>::init()
{
    m_array = new T[m_max_size];
    m_count = 0;
}

Here you are pre-allocating an array of T. This means that T must be default constructible (i.e. have a zero parameter constructor). Also if T is expensive to create and you don't use the whole array then you may be creating these object unnecessarily at an expensive point in the code.

You can allocate memory without calling the constructor then use placement new during the push to copy the data into the memory pool.

The same problem also appears in increase_array_size()

Potential Leak.

template <class T>
void MyStack<T>::increase_array_size()
{
    // STUFF

    for(int i = 0; i < m_count; i++)
        tmp[i] = m_array[i];           // If this throws an exception.
                                       // then you are leaking `tmp` pointer.

    // STUFF
}

Potential Invalid object.

template <class T>
void MyStack<T>::increase_array_size()
{
    // STUFF

    delete [] m_array;         // If this throws (because a T destructor throws)
                               // then you leave this object in an undefined
                               // state and you leak the `tmp` pointer.

    // STUFF
}

Use the copy and swap idiom to provide strong exception guarantee. This will also solve the problems in the last two points.

Provide Move Semantics.

This interface provides the standard copy semantics.

template <class T>
void MyStack<T>::push(T data)  // Also not passing by value causes a copy here
                               // So prefer to pass by const reference.
{
    if(m_count == m_max_size)
        increase_array_size();
    m_array[m_count++] = data;
}

But in C++11 we introduced move semantics which is potentially faster than copy semantics and Veridic template also allowing you to build the object in place.

template <typename T>
void MyStack<T>::push(T const& data);    // Copy data into MyStack.
template <typename T>
void MyStack<T>::push(T&& data);         // Move data into MyStack
template <typename... Args>
void MyStack<T>::emplace(Args&&... data);// Build data into MyStack

Assume the user knows the pre-conditions

You can check the pop() operation and throw. But if the user has already checked and knows that the stack is not empty you are doing extra work. For example the user is popping all the values from the stack in a for loop check for empty() in each iteration. Then your check becomes redundant extra work (because empty() was already called).

template <class T>
void MyStack<T>::pop()
{
    if(m_count == 0)
        throw std::underflow_error("Underflow Exception!!!");
    m_count--;

}

So in C++ you usually provide unchecked interface (let the user do the checking).

If you want you can also provide a checked interface then that is fine but usually not the default.

For example look at std::vector. Provides operator[](std::size_t index) for unchecked access to the data. But also provides at(std::size_t index) for checked access to the index.

Un-needed work.

Do you really need to call delete here?

template <class T>
void MyStack<T>::clear()
{
    delete [] m_array;
    m_max_size = m_initial_max_size;
    init();
}

You pre-initialized all the values initially. So you are not reallying on constructor/destructor properties. So this function can be simplified to:

template <class T>
void MyStack<T>::clear()
{
    m_count = 0;
}

If you want to release all the resources and re-create the default versions you can manually destroy all the objects.

Potential Leak.

template <class T>
void MyStack<T>::operator=(const MyStack<T> &rhs)
{
    if(this != &rhs)
    {
        delete [] m_array;                  // You should not delete the current
                                            // state until you know the operation
                                            // has succeeded. If the operation fails
                                            // you currently can not recover your state.

        init();
        for(int i = 0; i < rhs.m_count;i++)
        {
            this->push(rhs.m_array[i]);
        }

    }
}
#endif // MYSTACK_H