10
\$\begingroup\$

I have made an any class in C++, base loosely on boost::any, but written differently. I am checking to see if I have done it correctly and that there are no mistakes in it:

class any
{
public:
    any()
        : dt(new data<void *>(0))
        {
        }

    template<class T>
        any(const T &value)
        : dt(new data<T>(value))
        {
        }

    any(any &rhs)
        : dt(rhs.dt->duplicate())
        {
        }

    ~any()
        {
        delete dt;
        }

    template<class T>
        T cast() const
        {
        if (type() == typeid(T))
            {
            return (reinterpret_cast<data<T> *>(dt)->val);
            }
        throw std::exception("invalid cast type");
        }

    template<class T>
        operator T() const
        {
        return (cast<T>());
        }

    template<class T>
        bool is() const
        {
        return (type() == typeid(T));
        }

    any &operator=(any &rhs)
        {
        if (this != &rhs)
            {
            delete dt;
            dt = rhs.dt->duplicate();
            }
        return (*this);
        }

    template<class T>
        any &operator=(const T &value)
        {
        delete dt;
        dt = new data<T>(value);
        return (*this);
        }

    any &swap(any &rhs)
        {
        std::swap(dt, rhs.dt);
        return (*this);
        }

    template<class T>
        bool operator==(const T &value) const
        {
        return (type() == typeid(T) &&
            cast<T>() == value);
        }

    bool operator==(any &rhs) const
        {
        return (type() == rhs.type() && 
            dt->cmp(rhs.dt));
        }

    template<class T>
        bool operator!=(const T &value) const
        {
        return (!((*this) == value));
        }

    bool operator!=(any &rhs) const
        {
        return (!((*this) == rhs));
        }

    const std::type_info &type() const
        {
        return (dt->type());
        }

protected:
    struct dummy
        {
    public:
        virtual const std::type_info &type() const = 0;
        virtual bool cmp(dummy *rhs) const = 0;
        virtual dummy *duplicate() = 0;
        };

    template<class T>
        struct data
            : public dummy
        {
    public:
        data()
            : val()
            {
            }

        data(const T &value)
            : val(value)
            {
            }

        ~data()
            {
            }

        const std::type_info &type() const
            {
            return (typeid(T));
            }

        bool cmp(dummy *rhs) const
            {
            return (val == reinterpret_cast<data<T> *>(rhs)->val);
            }

        dummy *duplicate()
            {
            return (new data<T>(val));
            }

        T val;
        };

    dummy *dt;
    };
\$\endgroup\$
11
\$\begingroup\$

Your code is in pretty much good shape but there are still several issues apart from what mentioned by ChrisW:

  • There are many cases in input arguments and return types of functions where you are not particularly careful about const/non-const and value vs. reference.
  • This code won't work for built-in arrays, hence neither for C-style strings. One way is to decay the type of the input argument before storage; built-in arrays are decayed to pointers in this case (which means array elements are not really copied).
  • The default constructor shouldn't allocate anything; initialize the data pointer to nullptr and provide member functions empty() and clear() to control the state of having/not having data.
  • Your assignment operators are unnecessarily complex, inefficient (by self-tests) and not exception-safe (if new throws, the current object is already destroyed). The most elegant solution to all these issues is the copy-swap idiom, where all actual work is done by constructors alone.
  • You don't need typeid to test for type equality; a lightweight (but low-level) solution without RTTI is here.
  • Type identification should be kept as an internal detail; the minimal required functionality is type equality by is(); don't expose type(), rather keep it as private as possible.
  • Type checking is a good thing, but for performance you should also provide unchecked access.
  • Casting is from a base to a derived class, so need not (and should not) be done with reinterpret_cast; rather, dynamic_cast / static_cast for checked / unchecked access. dynamic_cast to a reference type will automatically throw an std::bad_cast if the object is not of the right type, so there is not need to manually check with is(). This does need RTTI but is more elegant.
  • Storing empty objects (like function objects) is currently inefficient, as it does need extra space on top of the virtual function table. This can be solved by the empty base optimization, which is done automatically by using std::tuple.
  • Implicit conversion operators are a possible source for ambiguities and confusion; you may keep them if you need their convenience, but use carefully (e.g. try to explicitly initialize an object of the right type).
  • Comparison operators are a clear overkill (if you have them, why not also have arithmetic operators, and so on?). If you still want them, define them as non-member functions, using public members is and cast to implement them.
  • There are no move semantics.
  • There is no specialized binary (non-member) function swap. Defaulting to std::swap is not as efficient as it involves three move operations; without move semantics, things are even worse as it involves three copy operations.

I took the liberty to re-factor your code to a great extent, and here is the result, resolving all issues above:

class some
{
    using id = size_t;

    template<typename T>
    struct type { static void id() { } };

    template<typename T>
    static id type_id() { return reinterpret_cast<id>(&type<T>::id); }

    template<typename T>
    using decay = typename std::decay<T>::type;

    template<typename T>
    using none = typename std::enable_if<!std::is_same<some, T>::value>::type;

    struct base
    {
        virtual ~base() { }
        virtual bool is(id) const = 0;
        virtual base *copy() const = 0;
    } *p = nullptr;

    template<typename T>
    struct data : base, std::tuple<T>
    {
        using std::tuple<T>::tuple;

        T       &get()      & { return std::get<0>(*this); }
        T const &get() const& { return std::get<0>(*this); }

        bool is(id i) const override { return i == type_id<T>(); }
        base *copy()  const override { return new data{get()}; }
    };

    template<typename T>
    T &stat() { return static_cast<data<T>&>(*p).get(); }

    template<typename T>
    T const &stat() const { return static_cast<data<T> const&>(*p).get(); }

    template<typename T>
    T &dyn() { return dynamic_cast<data<T>&>(*p).get(); }

    template<typename T>
    T const &dyn() const { return dynamic_cast<data<T> const&>(*p).get(); }

public:
     some() { }
    ~some() { delete p; }

    some(some &&s)      : p{s.p} { s.p = nullptr; }
    some(some const &s) : p{s.p->copy()} { }

    template<typename T, typename U = decay<T>, typename = none<U>>
    some(T &&x) : p{new data<U>{std::forward<T>(x)}} { }

    some &operator=(some s) { swap(*this, s); return *this; }

    friend void swap(some &s, some &r) { std::swap(s.p, r.p); }

    void clear() { delete p; p = nullptr; }

    bool empty() const { return p; }

    template<typename T>
    bool is() const { return p ? p->is(type_id<T>()) : false; }

    template<typename T> T      &&_()     && { return std::move(stat<T>()); }
    template<typename T> T       &_()      & { return stat<T>(); }
    template<typename T> T const &_() const& { return stat<T>(); }

    template<typename T> T      &&cast()     && { return std::move(dyn<T>()); }
    template<typename T> T       &cast()      & { return dyn<T>(); }
    template<typename T> T const &cast() const& { return dyn<T>(); }

    template<typename T> operator T     &&()     && { return std::move(_<T>()); }
    template<typename T> operator T      &()      & { return _<T>(); }
    template<typename T> operator T const&() const& { return _<T>(); }
};

I call it some instead of any because I prefer to hold something rather than anything :-) Plus, any is a common name for a function (like all), which is not the case for some.

Members cast() provide type-checked access, while members _() (shortest notation) provide unchecked access. I have chosen to give conversion operators unchecked access for performance but you are free to change that or remove them altogether.

I've also made a live example, including an extensive series of tests to demonstrate (almost) all possible uses.

Note that base (your dummy) now has a virtual destructor.

There is one more issue: using the free store is serious performance bottleneck when it comes to small types. What you can do is define a particular size to be allocated on stack, and only use the free store for larger objects, similar to short (or small) string optimization.

More generally, it is a good idea to parametrize the implementation with respect to how memory management (allocation, deallocation) is done by providing a kind of allocator object. any is really a container, even if of at most one object.

I have extended the implementation towards this direction but I will probably post this as a separate answer.

\$\endgroup\$
  • \$\begingroup\$ I eventually completed what I had promised and I posted it as a new question: Yet another 'any'. \$\endgroup\$ – iavr Apr 27 '14 at 15:39
5
\$\begingroup\$

I am checking to see if I have done it correctly and that there are no mistakes in it:

What have you done to test this class? You might want to write some unit-tests.


I think I see at least one bug: the destructor ...

    ~any()
    {
    delete dt;
    }

... calls delete dt.

dt seems to be of type dummy* ...

dummy *dt;

... but is actually of type data<T> which derives from dummy.

dummy is defined without a virtual destructor: so when you call delete dt then the dummy destructor will be called but the data destructor won't be called, and therefore the T data member of the data destructor won't be called, which is a bug if T has a non-trivial (non-default) destructor.


I don't see why you don't define duplicate as a const method, and define the any(any &rhs) constructor and the any &operator=(any &rhs) operator as taking const reference parameters.


You defined the implementation details of any as protected instead of private, as if you expect any to be subclassed. If any will be subclassed then its methods (e.g. its destructor) should perhaps be virtual.


You implemented operator T() const which is a conversion operator; but boost implements an explicit function i.e. any_cast for this purpose. I'm not sure why boost chose the latter but it may be better for some reason: perhaps it's safer because it's more explicit.

boost define a custom exception type bad_any_cast if the cast fails; you're just using a std::exception which might be harder to catch accurately.

\$\endgroup\$
  • \$\begingroup\$ boost uses the cast convention as they did not want automatic (compiler generated) conversion from an Any<T> to a T \$\endgroup\$ – Martin York Apr 23 '14 at 17:47
  • \$\begingroup\$ @LokiAstari Yes, but can you guess at why they didn't want automatic conversion? \$\endgroup\$ – ChrisW Apr 23 '14 at 18:20
  • \$\begingroup\$ At a guess: Auto conversion in general is a bad idea (obvious few exceptions). \$\endgroup\$ – Martin York Apr 23 '14 at 22:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.