Layer Stack class to practice std::shared_ptr

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
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 // std::vector
#include // std::shared_ptr std::weak_ptr std::dynamic_casting
#include // std::unordered_map
#include // assert
#include // std::is_pointer
#include // std::for_each
#include // 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 func

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 ` or , what you will use in , and 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

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.

StackExchange Code Review Q#58936, answer score: 5