How to adapt a lookup to use smart pointers?

[kitfox]

I’m trying to implement an index that I want to use to handle the nodes I encounter as I parse an xml document. I’ve been reading about smart pointers and was wondering how I might modify the below code so that it uses safer pointer management. Should I use unique_ptr for the Descriptor nodes? Should I return shared_ptr from the create() methods? Is there a way to do this without pointers? (The virtual members seem to prevent me from passing things by reference)

Is using smart pointers here overkill and I should just use raw pointers?

Any feedback on the design of the below code would be appreciated.

class Node
{
};

class RectNode : public Node
{
};

class CircleNode : public Node
{
};

class Descriptor
{
	std::string _name;
public:
	Descriptor(std::string& name) :
		_name(name)
	{}
	std::string& name() const { return _name; }
	virtual Node* build() = 0;
};

class RectNodeDescriptor : Descriptor
{
public:
	RectNodeDescriptor() : Descriptor(“rect”) {}
	Node* build() override { return new RectNode(); }
};

class CircleNodeDescriptor : Descriptor
{
public:
	CircleNodeDescriptor() : Descriptor(“circle”) {}
	Node* build() override { return new CircleNode(); }
};


class Index
{
	std::map<std::string, Descriptor*> descMap;
public:
	void addDescriptor(Descriptor* desc)
	{
		descMap[desc->name()] = desc;
	}
	Node* create(std::string& name)
	{
		if (descMap.contains(name))
			return descMap[name]->create();
		return nullptr;
	}
};

void main()
{
	Index index;
	index.addDescriptor(new RectNodeDescriptor());
	index.addDescriptor(new CircleNodeDescriptor());
	Node* node = index.create(“rect”);
}

[Thomas Huxhorn]

Right, never use raw pointer.
I don't see why you need any kind of pointer.
I even can't see what is the purpose Descriptor classes.

My quick and dirty try ;)

#include <iostream>
#include <string>
#include <vector>
#include <variant>


struct RectNode {
    std::string name() const {
        return "RectNode";
    }
};

struct CircleNode {
    std::string name() const {
        return "CircleNode";
    }
};

struct Node {
    std::variant<RectNode,CircleNode> v;

    // Helpfer function to hide some variant impl detailt.
    // This makes a copy of string name.
    // Really ugly.
    std::string name() const {
        std::string s;
        std::visit([&s](auto&& arg){ s = arg.name(); }, v);
        return s;
    }
};

class Index {
public:

    Node create(const std::string& name) {
        if(name == "rect") {
            return Node{RectNode()};
        } else if(name == "circle") {
            return Node{CircleNode()};
        } else {
            throw "shit";
        }
    }
};

int main() {
    Index index;

    Node node1 = index.create("rect");
    std::cout << node1.name() << "\n";

    Node node2 = index.create("circle");
    std::cout << node2.name() << "\n";
}

[JLBorges]

If pointers (along with an inheritance hierarchy) must be used, something along these lines, perhaps:

#include <iostream>
#include <string>
#include <memory>
#include <map>
#include <cassert>

class Node
{
    public:

        using pointer = std::unique_ptr<Node> ;

        virtual ~Node() = default ;
        // etc.
};

class RectNode : public Node { /* ... */ };

class CircleNode : public Node { /* ... */ };

class Descriptor
{
	std::string _name;

    public:

        using pointer = std::unique_ptr<Descriptor> ;

        explicit Descriptor( const std::string& name ) : _name(name) {}

        virtual ~Descriptor() = default ;
        // etc.

        const std::string& name() const { return _name; }

        virtual Node::pointer build() const = 0 ;
};

template < typename NODE_TYPE > class Descriptor_Impl : public Descriptor
{
    public:

        using Descriptor::Descriptor ;

        virtual Node::pointer build() const override
        { return std::make_unique<NODE_TYPE>( /* ... */ ) ; }
};

using RectNodeDescriptor = Descriptor_Impl<RectNode> ;
using CircleNodeDescriptor = Descriptor_Impl<CircleNode> ;

class Index
{
	std::map<std::string, Descriptor::pointer > descMap;

    public:

        void addDescriptor( Descriptor::pointer desc )
        {
            const auto& name = desc->name() ;
            if(desc) descMap.emplace( name, std::move(desc) ) ;
        }

        Node::pointer create( const std::string& name ) const
        {
            const auto iter = descMap.find(name) ;
            if( iter != descMap.end() ) return iter->second->build();
            else return nullptr ;
        }
};

int main()
{
    Index index;

    index.addDescriptor( std::make_unique<RectNodeDescriptor>( "rect" ) );
    index.addDescriptor( std::make_unique<CircleNodeDescriptor>( "circle" ) );

    auto rect = index.create( "rect" ) ;
    auto circ = index.create( "circle" ) ;
    auto bad = index.create( "bad" ) ;
    assert( rect && circ && !bad ) ;

    std::cout << "ok\n" ;
}

http://coliru.stacked-crooked.com/a/9768019306e97ae1

[malibor]

Your code will produce undefined behavior when derived class is being deleted because base class destructor will be called!
This is true even if deleting derived class trough base class pointer, in both cases base class destructor is called but has no implementation.

virtual Node* build() = 0;
this is bad, because even if the destructor is pure virtual you need to implement it, otherwise UB will happen in above mentioned cases.

This is correct:
virtual Node* build() = 0 { };

[kitfox]

Thanks for your suggestion, JLBorges - this looks like the best way to clean up what I wrote.

You asked if pointers and a hierarchy must be used, and my answer is I'm not sure. Using inheritance for the Node classes makes sense to me, since each node type refines the behavior of a more general node. The rest is just a means to an end. I need a way for my program to use some sort of token to refer to the different node types and create them on demand. Is there a way to design this with less reliance on pointers?

[JLBorges]

> I need a way for my program to use some sort of token to refer to the different node types
> and create them on demand. Is there a way to design this with less reliance on pointers?

No really convenient way to do that without dynamic object creation; use smart pointers.

You have the rudiments of the prototype (aka exemplar) design pattern.
https://en.wikipedia.org/wiki/Prototype_pattern

Here is a somewhat more stylised (idiomatic C++) version of the same.
(For brevity, in this toy example, all functions are implemented inline, with everything in one source file.)

#include <iostream>
#include <string>
#include <memory>
#include <vector>
#include <cassert>

class Node
{
    public:

        using pointer = std::unique_ptr<Node> ;

        virtual ~Node() = default ;
        // etc.

        static pointer create( const std::string& token  /*, ... */ )
        {
            // iterate through the prototypes asking each one
            // to create the node if the token matches its own unique token
            for( Node* p : prototype_list() )
            {
                auto ptr = p->do_create_from( token  /*, ... */ ) ;
                if(ptr) return ptr ; // return pointer to created node
            }

            std::cout << "unknown token '" << token << "' - no prototype accepts this token,"
                      << " no node was created\n" ;
            return nullptr ; // invalid token; no prototype accepted it
        }

    protected:

        struct this_is_the_exemplar{} ; // tag type to identify exemplar objects

        Node( /* ... */ ) = default ; // constructor for normal node objects

        Node( this_is_the_exemplar ) // constructor for prototype objects
        { prototype_list().push_back(this) ; } // add this prototype to the list of prototypes

        virtual pointer do_create_from( const std::string& token ) const = 0 ;

    private:

        static std::vector<Node*>& prototype_list()
        {
           // note: this takes care of timely dynamic initialisation of the vector
            static std::vector<Node*> prototypes ;
            return prototypes ;
        }
};


class RectNode : public Node
{
     public: RectNode( /* ... */ ) = default ; // constructor for normal RectNode objects

     protected: virtual pointer do_create_from( const std::string& token  /*, ... */  ) const override
     {
         if( token == unique_token ) // note: make this comparison case-insensitive if required
         {
             std::cout << "token == '" << token << "' creating a RectNode\n" ;
             return std::make_unique<RectNode>(  /* ... */ ) ; // clone the exemplar_object if required
         }
         else return nullptr ;
     }

     private:
         static const std::string unique_token ; // unique token for RectNode
         static const RectNode exemplar_object ; // prototype object for RectNode


         RectNode( this_is_the_exemplar e ) : Node(e) {}  // constructor for prototype RectNode object
};

const std::string RectNode::unique_token = "rect" ; // unique token for RectNode
const RectNode RectNode::exemplar_object{ Node::this_is_the_exemplar{} } ; // prototype object for RectNode

class CircleNode : public Node
{
     public: CircleNode( /* ... */ ) = default ; // constructor for normal CircleNode objects

     protected: virtual pointer do_create_from( const std::string& token  /*, ... */  ) const override
     {
         if( token == unique_token ) // note: make this comparison case-insensitive if required
         {
             std::cout << "token == '" << token << "' creating a CircleNode\n" ;
             return std::make_unique<RectNode>(  /* ... */ ) ; // clone the exemplar_object if required
         }
         else return nullptr ;
     };

     private:

         static const std::string unique_token ; // unique token for CircleNode
         static const CircleNode exemplar_object ; // prototype object for CircleNode

         CircleNode( this_is_the_exemplar e ) : Node(e) {}  // constructor for prototype CircleNode object
};

const std::string CircleNode::unique_token = "circle" ; // unique token for CircleNode
const CircleNode CircleNode::exemplar_object{ Node::this_is_the_exemplar{} } ; // prototype object for CircleNode

int main()
{
    auto rect = Node::create( "rect" ) ; // token == 'rect' creating a RectNode

    auto circ = Node::create( "circle" ) ; // token == 'circle' creating a CircleNode

    auto bad = Node::create( "bad" ) ; // unknown token 'bad' - no prototype accepts this token, no node was created

    assert( rect && circ && !bad ) ;
}

https://rextester.com/KADPN28526

[malibor]

@JLBorges Isn't kind of fun when people try to turn C++ syntax into C# syntax?