How bad is to use void pointer in std::vector decl

Question

I have two different classes as below:

class text
{ };

class element
{ };

And I want to store them in the class node:

template <typename T>
class node
{
    T cargo;

    std::vector<void*> children;

    node(T cargo) : cargo(cargo)
    { };

    void add_child(T node)
    {
        this->children.push_back((void*) node);
    }
}

So I would call the node this way storing both, text and element's:

element div;
text msg;

node<element> wrapper(div);

wrapper.add_child(msg);

EDIT: To get back the content I use T typedef type; and convert void pointer to (type*).

I know that's not very elegant nor functional, but I just can't figure out what's the correct way of doing that. So please tell me if this is practically acceptable and if it is not, how to do that in the proper manner.

Thanks in advance!

Answer 1

How would you get them back if you do this? From a void* there is no way to determine what is actually stored on the address.

Edit: If you always do a cast into T* then you can simply take T* as the parameter.

Answer 2

I would say that void* is nearly always "bad" (for some definition of bad). Certainly, there are likely to be better ways of expressing what it is you're trying to do. If it were me writing this code and I knew the types of the values I was going to put in, then I would consider using a Boost.Variant. If I didn't (for example, this was supplied as a library to someone else to "fill up"), then I would use Boost.Any

For example:

template <class T, class U>
struct node
{
    typedef boost::variant<T, U> child_type;

    std::vector<child_type> children;

    void add_child(T const &t)
    {
        children.push_back(t);
    }

    void add_child(U const &u)
    {
        children.push_back(u);
    }
};

...

node<text, element> n;
n.add_child(text("foo"));

A non-boost typed union solution:

struct node
{
    struct child
    {
        int type; // 0 = text; 1 = element

        union 
        {
            text*    t;
            element* e;
        } u;
    };

    std::vector<child> children;

    void add_child(text* t)
    {
        child ch;
        ch.type = 0;
        ch.u.t  = t;

        children.push_back(ch);
    }

    void add_child(element* e)
    {
        child ch;
        ch.type = 1;
        ch.u.e  = t;

        children.push_back(ch);
    }
};

Note: you have to be a whole lot more careful about memory management with the typed union.

Answer 3

If your container value is limited to a small number of types, you could achieve this using boost::variant as shown here:

#include <vector>
#include <boost/variant.hpp>

using namespace std;

class text
{ };

class element
{ };

template <typename T>
class node
{
    T cargo;

    static std::vector<boost::variant<text, element>> children;

    node(const T& cargo) : cargo(cargo)
    { };

    void add_child(const T& node)
    {
        children.push_back(boost::variant<text, element>(node));
    }
};

I have taken the liberty of suggesting a couple of other mods - use const reference instead of pass-by-value on node constructor and add_child; make the container children static as I don't think it makes sense for each node<T> to have its own container. Locking would be required for multithreaded usage of add_child in this case. These comments apply whether you can use Boost or not in your final solution.

You can perform operations on the vector elements using either get or static_visitor - the latter is preferable since you can make this generic - as shown here. An example of vector iteration analogous to what you would use for this solution:

class times_two_generic
    : public boost::static_visitor<>
{
public:

    template <typename T>
    void operator()( T & operand ) const
    {
        operand += operand;
        cout << operand << endl;
    }

};

std::vector< boost::variant<int, std::string> > vec;
vec.push_back( 21 );
vec.push_back( "hello " );

times_two_generic visitor;
std::for_each(
      vec.begin(), vec.end()
   , boost::apply_visitor(visitor)
   );

Output is:

42

hello hello

Answer 4

First, there's no such a thing as "bad" to use a void pointer. Forget all the conventions and bla-blas, and do what's most appropriate for your case.

Now, in you specific case, if there's any connection between those two classes - you may declare a base class, so that those two will inherit it. Then, you may declare the vector of a pointer of that base class.

Answer 5

#include <vector>
using namespace std;

class Element {};
class Text {};
class Nothing {};

class Node
{
private:
    vector< Node* >     children_;
protected:
    Node() {}
public:
    void add( Node* p ) { children_.push_back( p ); }
    virtual ~Node() {}
};

template< class Cargo >
class CargoNode
    : public Node
{
private:
    Cargo   cargo_;
public:
    CargoNode(): cargo_() {}
};

typedef CargoNode< Element >    ElementNode;
typedef CargoNode< Text >       TextNode;
typedef CargoNode< Nothing >    RootNode;

int main()
{
    RootNode*   root    = new RootNode;

    root->add( new ElementNode );
    root->add( new ElementNode );
    root->add( new TextNode );
    root->add( new ElementNode );   
    // Etc.
}

Cheers & hth.,

PS: Error checking, lifetime management, iteration etc. omitted in this example code.

Answer 6

Define a shared base class for element and text and then add_child can take a pointer to the base class, and the vector can store pointers to the base class.