Layer Stack class to practice std::shared_ptr

Question

The following three source files is to define and test a class StackLayer.

While it was written in a need for scalable layer-based architecture design, it was also a practice for std::shared_ptr (I'm new to C++11).

I'm not sure if I'm understanding the concept and best practices for std::weak_ptr and std::shared_ptr.

You will notice that the LayerNode struct is completely public to derived classes, which I couldn't avoid because of some error std::shared_ptr was complaining about ("conversion exists but inaccessible"). I don't like this because it ruins all the encapsulation.

I'm also not sure if I have any memory leak or not (at least it is not breaking).

To prevent lower layer to access higher layer, I also had to make GetLayer function return null if that case happens. Should I assert or throw on this case instead of returning nullptr?

I only tested it in Visual C++ CPT120 (might have missing 'typename' errors in gcc, or not if I get lucky)

Singleton.h

template<typename Derived>
class Singleton
{
public:
    // Getter
    static Derived& GetInstance( void )
    {
        static Derived s_instance;
        return s_instance;
    }
protected:
    // Ctor
    Singleton( void ){}
    // Dtor
    virtual ~Singleton( void ){}
private:
    // NO COPY
    Singleton( const Singleton& rhs );
    // NO ASSIGNMNET
    Singleton& operator=( const Singleton& rhs );
}; // class Singleton

LayerStack.h

#ifndef LAYER_STACK_INCLUDE_GUARD
#define LAYER_STACK_INCLUDE_GUARD
#include "Singleton.h"      // Singleton
#include <vector>           // std::vector
#include <memory>           // std::shared_ptr std::weak_ptr std::dynamic_casting
#include <unordered_map>    // std::unordered_map
#include <cassert>          // assert
#include <type_traits>      // std::is_pointer
#include <algorithm>        // std::for_each
#include <functional>       // std::function

class LayerStack;

// public Inherit this class to make a layer.
// layer is to be added to an instance of LayerStack before calling GetLayer function.(Look at LayerStack::Push function)
template <typename Derived>
class Layer : public LayerStack::LayerNode, public Singleton<Derived>
{
    friend class LayerStack;

public:
    typedef Derived instance_type;
    Layer( void )
        : LayerNode( typeid(Derived).hash_code() )
        , Singleton()
    {
    }
    template <typename T>
    std::shared_ptr<T> GetLayer( void )
    {
        return LayerNode::m_pLayerStack->QueryLayerFrom<T>(m_attitue);
    } // std::shared_ptr<T> GetLayer( void )
    virtual ~Layer( void ) {}
}; // class Layer : public LayerStack::LayerNode, public Singleton<Derived>

// Layer Stack.
// Can store instance of layer inheriting class Layer.
// These rules are to be followred:
//      * Each type of layer can only appear once in the same layer stack.( Layer is Singleton. Look at Layer definition )
//      * Layer can access other layer by calling GetLayer function, but not in ascending order( Only layer pushed eairler than caller layer can be accessed. In other words, top layer can acess all layers, while bottom layer cannot access any layer)
class LayerStack
{
    template <typename Derived>
    friend class Layer;
private:
    typedef std::size_t type_hash_type;
    typedef std::size_t size_type;
    struct LayerNode
    {
        LayerNode( type_hash_type typeHash )
            : m_pLayerStack(nullptr)
            , m_typeHash(typeHash)
            , m_attitue(0)
        {
        }
        virtual ~LayerNode( void ) {}
        LayerStack*                 m_pLayerStack;
        LayerStack::type_hash_type  m_typeHash;
        int                         m_attitue;
    };
public:
    typedef std::unordered_map<type_hash_type, std::weak_ptr<LayerNode>> layer_map_type;
    typedef std::vector<std::shared_ptr<LayerNode>> layer_vec_type;
private:
    layer_map_type m_layerMap;
    layer_vec_type m_layerVec;
public:
    LayerStack( void )
        : m_layerMap()
        , m_layerVec()
    {
    }
    template <typename T>
    void Push( std::shared_ptr<T>&& item )
    {
        static_assert(!std::is_pointer<T>::value, "Cannot use a shared pointer to a pointer");
        // Gets type hash code
        type_hash_type h = typeid(T).hash_code();
        // Search item by the hash code from the map.
        // item shouldn't be duplicate(Singleton)
        assert(m_layerMap.find(h) == m_layerMap.end());
        // Set Parent pointer
        item->m_pLayerStack = this;
        // Add item to the vector
        m_layerVec.push_back(item);
        // Add item to the map
        m_layerMap[h] = item;
        // Set attitue
        item->m_attitue = m_layerVec.size();
    } // void Push( std::shared_ptr<T>&& item )
    void Pop( void )
    {
        m_layerMap.erase(m_layerVec.back()->m_typeHash);
        m_layerVec.pop_back();
    } // void Pop( void )
    template <typename CAST_TYPE>
    void ForAscendingEach( std::function<void(CAST_TYPE*)> callback)
    {
        std::for_each( m_layerVec.begin(), m_layerVec.end(), [&]( std::shared_ptr<LayerNode> spNode )
        {
            callback(std::dynamic_pointer_cast<CAST_TYPE>(spNode).get());
        });
    }
    template <typename CAST_TYPE>
    void ForDescendingEach( std::function<void(CAST_TYPE*)> callback)
    {
        std::for_each( m_layerVec.rbegin(), m_layerVec.rend(), [&]( std::shared_ptr<LayerNode> spNode )
        {
            callback(std::dynamic_pointer_cast<CAST_TYPE>(spNode).get());
        });
    }
    template <typename CAST_TYPE, typename ...Args>
    void ForAscendingEach( void (CAST_TYPE::*callback)(Args...) , Args... args)
    {
        std::for_each( m_layerVec.begin(), m_layerVec.end(), [&]( std::shared_ptr<LayerNode> spNode )
        {
            (std::dynamic_pointer_cast<CAST_TYPE>(spNode).get()->*callback)(args...);
        });
    }
    template <typename CAST_TYPE, typename ...Args>
    void ForDescendingEach( void (CAST_TYPE::*callback)(Args...) , Args... args)
    {
        std::for_each( m_layerVec.rbegin(), m_layerVec.rend(), [&]( std::shared_ptr<LayerNode> spNode )
        {
            (std::dynamic_pointer_cast<CAST_TYPE>(spNode).get()->*callback)(args...);
        });
    }
    size_type Size( void )
    {
        return m_layerVec.size();
    } // size_type Size( void )
    template <typename T>
    std::shared_ptr<T> QueryLayer( void )
    {
        assert( m_layerMap.find(typeid(T).hash_code()) != m_layerMap.end() );

        return std::dynamic_pointer_cast<T>(m_layerMap[typeid(T).hash_code()].lock());
    }
private:
    template <typename T>
    std::shared_ptr<T> QueryLayerFrom( int attitue )
    {
        assert( m_layerMap.find(typeid(T).hash_code()) != m_layerMap.end() );

        return m_layerMap[typeid(T).hash_code()].lock()->m_attitue < attitue
            ? std::dynamic_pointer_cast<T>(m_layerMap[typeid(T).hash_code()].lock())
            : nullptr; 
    }
}; // class LayerStack

Main.cpp

#include "LayerStack.h"
#include <iostream>

#define PROMPT(MSG) do { std::cout << " * " << (MSG) << std::endl; } while(0)
#define SHOW(EXPR) do { std::cout << (#EXPR) << " : " << (EXPR) << std::endl; } while(0)

class Base
{
public:
    virtual ~Base( void ){}
    virtual void Print( void ) = 0;
    virtual void PrintInt( int i ) = 0;
};

class A : public Layer<A>, public Base
{
public:
    virtual void Print( void ) override
    {
        std::cout << "A Printing" << std::endl;
    }
    virtual void PrintInt( int i ) override
    {
        std::cout << "A Printing " << i << std::endl;
    }
};

class B : public Layer<B>, public Base
{
public:
    virtual void Print( void ) override
    {
        std::cout << "B Printing" << std::endl;
    }
    virtual void PrintInt( int i ) override
    {
        std::cout << "B Printing " << i << std::endl;
    }
};

void PrintBase( Base* pBase )
{
    pBase->Print();
}

int main( void )
{
    LayerStack stack;

    PROMPT("Pushing A");
    stack.Push( std::make_shared<A>() );
    SHOW(stack.Size());

    PROMPT("Pushing B");
    stack.Push( std::make_shared<B>() );
    SHOW(stack.Size());

    PROMPT("Now Stack Looks like:");
    PROMPT("  [Top] B");
    PROMPT("  [Bottom] A");

    PROMPT("Query A Directly from the stack layer");
    std::shared_ptr<A> sptra;
    SHOW(sptra = stack.QueryLayer<A>());

    PROMPT("Query B Directly from the stack layer");
    std::shared_ptr<B> sptrb;
    SHOW(sptrb = stack.QueryLayer<B>());

    PROMPT("Call function in ascending order");
    stack.ForAscendingEach<Base>(PrintBase);

    PROMPT("Call function in descending order");
    stack.ForDescendingEach<Base>(PrintBase);

    PROMPT("Call member function in ascending order");
    stack.ForAscendingEach<Base>(&Base::PrintInt, 1);

    PROMPT("Call member function in descending order");
    stack.ForDescendingEach<Base>(&Base::PrintInt, 2);

    PROMPT("Accessing A from B should success(A is lower than B.(A is pushed eairlier) descending access is allowed)");
    SHOW(sptra = sptrb->GetLayer<A>());

    PROMPT("Accessing B from A should fail(B is higher than A.(B is pushed later) ascending access is prevented.)");
    SHOW(sptrb = sptra->GetLayer<B>());

    PROMPT("Popping A");
    stack.Pop();
    SHOW(stack.Size());

    PROMPT("Popping B");
    stack.Pop();
    SHOW(stack.Size());

    return 0;
} // int main( void )

Output:

 * Pushing A
stack.Size() : 1
 * Pushing B
stack.Size() : 2
 * Now Stack Looks like:
 *   [Top] B
 *   [Bottom] A
 * Query A Directly from the stack layer
sptra = stack.QueryLayer<A>() : 01009074
 * Query B Directly from the stack layer
sptrb = stack.QueryLayer<B>() : 0100D544
 * Call function in ascending order
A Printing
B Printing
 * Call function in descending order
B Printing
A Printing
 * Call member function in ascending order
A Printing 1
B Printing 1
 * Call member function in descending order
B Printing 2
A Printing 2
 * Accessing A from B should success(A is lower than B.(A is pushed eairlier) de
scending access is allowed)
sptra = sptrb->GetLayer<A>() : 01009074
 * Accessing B from A should fail(B is higher than A.(B is pushed later) ascendi
ng access is prevented.)
sptrb = sptra->GetLayer<B>() : 00000000
 * Popping A
stack.Size() : 1
 * Popping B
stack.Size() : 0

Answer 1

Some minor things:

• Include guard names don't need an _INCLUDE_GUARD at the end. They should not be confused with macros anyway, although you won't have very many of them in C++.

• Consider organizing your library #includes, such as alphabetically. This will make it easier to keep track of all of them.

• Comments like //Ctor and //Dtor are noise since it's already obvious. Comments shouldn't point out the obvious; they should provide useful information.

• Since your types are named in PascalCase, name your functions in camelCase or snake_case. It's generally good to use separate naming types for these two things.

• All the std::endls can be replaced with "\n". You don't need to flush the buffer so many times (which is what the former also does), and the latter will just output a newline.

• You don't need an explicit return 0 at the end of main(). Successful termination is already implied at this point, so the compiler will do this return for you here.

• You don't need a comments after a closing brace, especially at the end of main(). It may be okay if you have many nested ones, but it's just clutter for others.

• Unlike in C, a void parameter is not needed for functions taking no arguments.

Answer 2

I have a few things to add to what @Jamal already mentioned:

• If you want a class to be uncopyable, don't make its copy constructor and assignment operator private. Instead, mark them as deleted functions:

  Singleton( const Singleton& rhs ) = delete;
  Singleton& operator=( const Singleton& rhs ) = delete;
  

  The = delete is explicit by itself, so I guess that you can also remove the comments about the class not being copyable.

• I don't think that you will use Singleton as a base class for polymorphism (you won't create Singleton* instances to store derived class instances), so you don't have to make it destructor virtual. Generally speaking, virtual destructors are useful when you need to delete a instance via a base class pointer.

• I don't think that you need to specify what you use in each included header. While it might be useful for <memory> or <algorithm>, what you will use in <vector>, <unordered_map> and <cassert> is totally obvious.

• It might be a good idea to forget the old typedefs and use the brand new type aliases instead. Their advantages are multiple: the syntax is closer to a variable declaration and to a namespace alias, and it can be templated to create an alias template. Using type aliases will help you to write consistent code:

  using instance_type = Derived;
  

• This kind of "closing comment" is not really useful:

  } // class Layer : public LayerStack::LayerNode, public Singleton<Derived>
  

  A good indentation will provide all the information you need. Moreover, IDEs and code editors generally highlights matching braces, further reducing the need for this kind of comments. You shouldn't write too many comments that are somehow obvious or uneeded, for after a modification of the code, a comment may lie and a lying comment is worse than no comments at all.

• When a method does not modify the enclosing class, you should const-qualify it:

  size_type Size( void ) const
  {
      return m_layerVec.size();
  }
  

  There are many functions in your code that do not modify their classes and that ought to be const-qualified.