Is this a good way of using std::shared_ptr?

Question

Can this code be considered good design?

It compiles and works fine, both with GCC and Visual Studio. The slot object ends up really small, neatly packed and easy to reason about.

However, how much the casting will slow down my program in the end?

If I ended up using boost::any or boost::variant I would still put them in a std::shared_ptr because I do need it to be a smart pointer.

I'm using C++14.

//EDITED BEGIN

Context: I'm builing an interpreter, something in the vein of lisp / ruby. I want slot objects to be a sort of god object. I want to be able to construct and return new slot objects inside the slot object itself, both shallow and deep copies. The data in the slot object is a pointer because I intend that data to be shared between objects. The slot_t enumerator exists mainly to be used in switch statements and it needs to be accessible outside the slot class, that's why it was made global. The slot_t in the constructor is needed because sometimes a type can have the same internal representation, and thus a mean to disambiguation is needed. The example I gave was rushed and it does have some problems.

This is how I envision my slot object:

4 members -> gate (variable / constant, ...), type, data and docstring.
Operator overloads -> bool, ==, !=, <, > <=, >=.
Some methods: copy, deep_copy, get_type(by returning a new slot), etc...
I think you did get the point. :D

This is my rushed example:

//EDITED END

#include <iostream>
#include <memory>
#include <sstream>
#include <string>

using std::cout;
using std::endl;

enum class slot_t {
    number_t,
    string_t
};

class slot {
public:
    slot_t type;
    std::shared_ptr<void> data;
    slot(slot_t const p_type, double long const & p_data)
        : type {p_type}
        , data {std::make_shared<double long>(p_data)}
    {}
    slot(slot_t const p_type, std::string const & p_data)
        : type {p_type}
        , data {std::make_shared<std::string>(p_data)}
    {}
    std::string get_type() const {
        std::ostringstream output;
        switch (type) {
            case slot_t::string_t: output << "String: " << as<std::string>(); break;
            case slot_t::number_t: output << "Number: " << as<double long>(); break;
        }
        return output.str();
    }
    template <typename t>
    t as() const {
        return *std::static_pointer_cast<t>(data);
    }
};

int main() {
    slot hello {slot_t::number_t, 123};
    slot world {slot_t::string_t, "Hello, world!"};

    cout << hello.as<double long>() << endl;
    cout << world.as<std::string>() << endl;

    cout << hello.get_type() << endl;
    cout << world.get_type() << endl;
    return 0;
}

Answer 1

This really looks like a job for variant. In effect, what you have written is a poorly written shared variant.

There are a number of problems. The first of which is that you pass the type separately from the type in the ctor. The second is you have poor visiting code.

So some tweaks:

// sink variables: take by value, move into storage:
slot(double long p_data)
    : type {slot_t::number_t}
    , data {std::make_shared<double long>(p_data)}
{}
// sink variables: take by value, move into storage:
slot(std::string p_data)
    : type {slot_t::string_t}
    , data {std::make_shared<std::string>(std::move(p_data))}
{}
// get type from the type, not from a separate variable.

// boost and std style visit function:
template<class F>
auto visit( F&& f )
-> typename std::result_of< F(int&) >::type
{
  switch(type) {
    case slot_t::string_t: return std::forward<F>(f)( as<std::string>() );
    case slot_t::number_t: return std::forward<F>(f)( as<double long>() );
  }
}
// const visit:
template<class F>
auto visit( F&& f ) const
-> typename std::result_of< F(int const&) >::type
{
  switch(type) {
    case slot_t::string_t: return std::forward<F>(f)( as<std::string>() );
    case slot_t::number_t: return std::forward<F>(f)( as<double long>() );
  }
}
// const and non-const as that return references:
template <typename t>
t const& as() const {
    return *static_cast<t const*>(data.get());
}
template <typename t>
t& as() {
    return *static_cast<t*>(data.get());
}

Near enum type_t:

inline std::string get_typename(type_t type) {
  switch (type) {
    case type_t::string_t: return "String";
    case type_t::number_t: return "Number";
  }
}

as name is a property of type_t, not of your slot type.

A C++14 implementation of get_type, where we create a visitor as a lambda:

std::string get_type() const {
  std::ostringstream output;
  output << get_typename(type) << ": ";
  return visit( [&](auto&& value) {
    output <<  value; // notice not casting here
  } );
  return output.str();
}

This visit pattern mimics how boost::variant works, and is what you should pattern your code after. In C++11 you do have to write your visitor class separately, not as a lambda.

Of course:

struct slot {
  std::shared_ptr< boost::variant<std::string, long double> > pImpl;
};

is a much better chassy. Now we just have to map your enums and operations over to operating on the variant inside the shared ptr.

However, another rule of thumb is that shared state is bad. It is almost as bad as global state; it makes programs hard to reason about. I'd do away with the shared state, and replace it with a naked variant.

Both ints and long doubles are cheap to copy. A variant containing them is also cheap to copy.

---

visit is powerful. But sometimes you want completely different code based on the type of the object. This works:

template<class...Ts>
struct overload_t {
private:
  struct never_used {};
public:
  void operator()(never_used)=delete;
};
template<class T0, class...Ts>
struct overload_t: T0, overload_t<Ts...> {
  overload_t(T0 t0, Ts...ts):
    T0(std::move(t0)),
    overload_t<Ts...>(std::move(ts)...)
  {}
  overload_t(overload_t&&)=default;
  overload_t(overload_t const&)=default;
  overload_t& operator=(overload_t&&)=default;
  overload_t& operator=(overload_t const&)=default;

  using T0::operator();
  using overload_t<Ts...>::operator();
};
template<class...Fs>
overload_t<Fs...>
overload(Fs...fs) {
  return {std::move(fs)...};
}

now we build overloads:

auto detect_type = overload(
  [](std::string const&){ std::cout << "I am a string\n"; },
  [](long double){ std::cout << "I am a number\n"; }
};
slot.visit(detect_type);

and overload resolution kicks in, resulting in the right type-safe function being called.

If you want to ignore some types, just do this:

auto detect_type = overload(
  [](std::string const&){ std::cout << "I am a string\n"; },
  [](auto const&){ std::cout << "I was ignored\n"; }
};

and again, let overload resolution solve your problems.

Isolate your type-unsafe operations to a narrow part of your code base. Do not force users to get types exactly right or generate undefined behavior every time they interact with your variant type.

Answer 2

Really have to question your design choices. So your first question, "Can this code be considered good design?" I have to say probably not. Public variables are bad, m-kay? Your class is a slut.

The cost of creating a shared_ptr in your as function is going to be about the only cost you have. You could get rid of it by using get and static_cast rather than static_pointer_cast. The static_cast is performed at compile time and should have no runtime impact.

In the end though you need to profile to answer your question. Anything else is an assumption.