2

Below is the (very) stripped down code of a implementation.

The library code is as follows:

#pragma once
#include <iostream>
#include <map>
#include <string>

// Necessary as interface and due to QObject Macro.
class Base
{
  public:
    Base( const std::string& name ) : name_( name )
    {
    }

    virtual ~Base(){}

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

  private:
    std::string name_;
};

template < typename Derived, typename ObjectType >
class TemplatedBase : public Base
{
  public:
    TemplatedBase( const std::string& name ) : Base( name )
    {
    }

    ObjectType object()
    {
        return object_;
    }

    ObjectType object_;
};

class DerivedA : public TemplatedBase< DerivedA, int >
{
  public:
    DerivedA( const std::string& name ) : TemplatedBase< DerivedA, int >( name )
    {
    }
};

class DerivedB : public TemplatedBase< DerivedB, float >
{
  public:
    DerivedB( const std::string& name ) : TemplatedBase< DerivedB, float >( name )
    {
    }
};

class Container
{
  public:
    template < typename T >
    void addToMap( T& map_object )
    {
        const std::string name = map_object.name();
        // ASSERT( map_.find( name ) == map_.end() );
        map_.emplace( std::make_pair( name, &map_object ) );
    }

    template < typename T >
    auto getObject( std::string name ) -> decltype( ( ( T* )nullptr )->object() )
    {
        auto search = map_.find( name );
        // How can this dynamic_cast be avoided? 
        T* ptr      = dynamic_cast< T* >( search->second );
        // ASSERT( ptr == nullptr );
        return ptr->object();
    }

    std::map< std::string, Base* > map_;
};

With an example usage of:

int main( int argc, char* argv[] )
{
    Container container;

    DerivedA a( "Name_A" );
    DerivedB b( "Name_B" );

    container.addToMap( a );
    container.addToMap( b );

    // How can I avoid to specify the type in the map as template?
    auto object_a = container.getObject< DerivedA >( "Name_A" );
    auto object_b = container.getObject< DerivedB >( "Name_B" );
}

Some explanation for the code:

  • The Base Class is necessary because the Q_OBJECT Macro requires a non-template class as well as for the interface.
  • The templated Base class has some sort of lazy copy implemented, as the objects stored are relativly large in the real application.
  • The Container is made available for an arbitrary number of threads and the lazy copy of the TemplatedBase Class is handling the thread safety.
  • The Container is used in a context that doesn't know (nor need to know) the Derived Classes themself.

While the code itself is working fine, I am looking for a different design that makes the dynamic_cast and the type specification during readout unnecessary.

I have tried several types of type erasure (boost::type_erasure, boost::variant) and different container types. However I always ran into the problem of different return types of the object() function.

Trevir
  • 1,253
  • 9
  • 16
  • 3
    "While the code itself is working fine" you're looking for a code review then. http://codereview.stackexchange.com/ – UKMonkey Oct 18 '16 at 12:40
  • I didn't know which is more appropriate. If necessary, the question can be moved. – Trevir Oct 18 '16 at 12:41
  • If you are using Qt (as implied by the mention of `Q_OBJECT`, why don't you just use `QVariant`? – SingerOfTheFall Oct 18 '16 at 12:43
  • @SingerOfTheFall `QVariant` has the problem of different return types in the `object()` function – Trevir Oct 18 '16 at 12:44
  • @UKMonkey I'm not so sure. OP has a specific question which is how to avoid the `reinterpret_cast`. – Chris Drew Oct 18 '16 at 12:46
  • can you just not use a `dynamic_cast` ? – Hayt Oct 18 '16 at 12:48
  • 1
    @Trevir, you can't avoid that. You _will_ have to cast to a particular type yourself, at one point or another. – SingerOfTheFall Oct 18 '16 at 12:48
  • Is base derived from QObject? because you need that for the macro AFAIK. – Hayt Oct 18 '16 at 12:51
  • @Hayt: Yes it is, dynamic_cast should be possible. However I still need to specify the type twice. – Trevir Oct 18 '16 at 12:53
  • If you want a map which can store different types this is what comes with it. Once while inserting once while reading. Maybe the "cleanest" here would be to not use templates at all and just return a `Base*` and the client code can take care of the conversion. – Hayt Oct 18 '16 at 12:54
  • Or is it alternatively ok to have 2 containers? one For `DerivedA` and one for `DerivedB`. Then you don't lose the types. – Hayt Oct 18 '16 at 12:56
  • @Hayt this is a pretty common way to do things for types in an inheritence tree. There's nothing wrong iwth it, except for the reinterpret cast not being a dynamic_cast – xaxxon Oct 18 '16 at 12:58
  • @Hayt: Sadly not. There are already several Derived Types (and still counting) with hundreds of possible ObjectTypes. – Trevir Oct 18 '16 at 12:58
  • I mean you can change the template so you have `container` which can only store `DerivedA`. If not you are losing the type and have to re-specify it. So provide it twice is the only option. – Hayt Oct 18 '16 at 13:03
  • @Hayt: Yes that would be possible, however this leads to several thousands of different containers. – Trevir Oct 18 '16 at 13:15
  • well with your solution you will have several thousand `addToMap` functions etc in one container. – Hayt Oct 18 '16 at 13:18
  • @Hayt: Yes, but I don't need to type them. Is it possible to have these kinds of containers generated automatically? And to access the right container during runtime? I'm not afraid of boilerplate as long as the consumer code gets cleaner! – Trevir Oct 18 '16 at 13:21
  • to access the right container dynamically during runtim? not really. The issue here is types are needed for compile time. When you want something to be done on runtime when you don't know the types you can not really avoid dynamic cast and having to specify the type here multiple times. You could maybe workaround this with a script which generates the code for you though. – Hayt Oct 18 '16 at 13:30
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/126016/discussion-between-trevir-and-hayt). – Trevir Oct 18 '16 at 13:32

2 Answers2

2

You cannot avoid putting the type name here if you want to return a specific type other than Base:

    auto object_a = container.getObject< DerivedA >( "Name_A" );

There's no way for the compiler to know what to return at compilation time since the value won't be known until runtime.

As for your reinterpret cast, you're right to not like it. That needs to be a dynamic_cast or you must build some other way to tell what type your objects are at runtime into your system. Also, you probably meant static_cast not reinterpret cast. However, static_cast is also not correct if you don't absolutely know that every lookup will specify the correct corresponding type as a template parameter that matches the actual runtime type of the stored object.

Right now, if you make a bad call, your system will result in undefined behavior. Your code is "working fine" because you're not calling getObject with an incompatible templated type, but as soon as you do, your program is likely to not be working fine.

xaxxon
  • 19,189
  • 5
  • 50
  • 80
  • That's why I want to get rid of the `reinterpret_cast`. After all I specified the type already at the construction of the container. – Trevir Oct 18 '16 at 12:49
  • @Hurkyl dynamic_cast doesn't have anything to do with virtual inheritence. However, it does require a polymorphic type, which requires a virtual functino. (comment since deleted0 – xaxxon Oct 18 '16 at 12:50
  • @Trevir the problem is the code doesn't know if your request is valid. If you said ` auto object_a = container.getObject< DerivedA >( "Name_B" ); ` (note I swietched the string at the end from Name_A to Name_B) your code would return a value, but your program would have undefined behavior because it's NOT a DerivedA, it's a DerivedB. – xaxxon Oct 18 '16 at 12:51
  • @xaxxon you don't know/see this but when it is derived from QObject (which it is just not shown) it has virtual functions. It is just stripped away form the example. – Hayt Oct 18 '16 at 12:55
  • Yes, sorry for the obfuscation. Dynamic_cast is working just fine. – Trevir Oct 18 '16 at 12:56
  • @Trevir changing the reinterpret cast to dynamic cast, I believe your code is correct (it will throw an exception if you request an incompatible type) and about as syntactically tight as it can be. – xaxxon Oct 18 '16 at 12:57
  • “However, `static_cast` is also not correct” — Care to elaborate? You *can* use `static_cast` here — the difference to `dynamic_cast` is that you won’t get any diagnostics if you’re accidentally casting the wrong type, and the result is UB. But assuming that there’s no chance of misuse, a `static_cast` would be entirely fine here. Since OP’s code doesn’t check the result of the cast anyway there’s *no reason* to use a `dynamic_cast` here (the resulting dereferencing of the pointer is UB anyway), and consequently a `static_cast` *should* be used. – Konrad Rudolph Oct 18 '16 at 13:00
  • @KonradRudolph he almost certainly wants to use this in cases where he cannot know at compile time what the resulting type will be. And by "change to use dynamic_cast" I meant.. actually use it correctly. – xaxxon Oct 18 '16 at 13:07
  • @xaxxon That’s irrelevant, since the type needs to be statically specified anyway, as your answer correctly says. In fact, for the purpose of OP’s question there’s no difference between `static_cast` and `dynamic_cast` (except that the latter implies that the result is checked by the user, which isn’t the case here). – Konrad Rudolph Oct 18 '16 at 13:08
  • @KonradRudolph Yes, he has to use dynamic_cast correctly. – xaxxon Oct 18 '16 at 13:09
  • Thanks to all, I changed the question to make the virtual chain more visible and used the dynamic_cast instead. However the main question remains until someone comes along who has a genius idea or it isn't possible after all. – Trevir Oct 18 '16 at 13:13
  • @KonradRudolph it's a very common approach to say how you want the object back and to test to see if it is actually of that type and either throw or return nullptr if it's not. Or use an Optional. – xaxxon Oct 18 '16 at 13:13
  • @Trevir you arne't calling dynamic_cast in a useful way because you aren't checking to see if it returns `nullptr`. – xaxxon Oct 18 '16 at 13:14
  • @xaxxon I’m well aware of that, and in such a case you’d indeed need to use `dynamic_cast`. However, this is simply not what the OP is doing. (EDIT: seeing your last comment it seems we’re actually in agreement here.) – Konrad Rudolph Oct 18 '16 at 13:14
  • @KonradRudolph assuming a new user's testcase is exactly what they mean it to be isn't often the fastest way to get the where they want to be. – xaxxon Oct 18 '16 at 13:15
  • @xaxxon Yes, exactly right. I think your answer’s last paragraph eluded me before. – Konrad Rudolph Oct 18 '16 at 13:15
1

Let’s consider what exactly you’re doing here:

  • You are casting
    • a pointer of a base class to a pointer of its derived child class,
    • in a non-virtual inheritance chain,
    • without casting away const-ness.
  • You are not checking the result’s correctness.

Consequently, the correct cast to use here is static_cast.

However, this will break (= result in undefined behaviour) in cases where the user accidentally calls container.getObject with a mismatching type. Consequently, you should be checking the cast result’s correctness, as explained in xaxxon’s answer.

And no, there’s no way of avoiding to specify the target type in the cast. You must specify a static type to an object in order to work with it, since the result of ptr->object() isn’t type-erased. Even boost::variant requires users to specify static types whenever they actually want to access the stored object (e.g. in the signature of a visitor).

Community
  • 1
  • 1
Konrad Rudolph
  • 530,221
  • 131
  • 937
  • 1,214
  • Thanks, I made the virtual inheritance more visible. – Trevir Oct 18 '16 at 13:11
  • @Trevir That’s not [virtual inheritance](https://en.wikipedia.org/wiki/Virtual_inheritance). Virtual inheritance occurs when you actually make the base class virtual (`class A : public virtual B`), which is fundamentally different from having virtual functions. – Konrad Rudolph Oct 18 '16 at 13:13
  • Sorry, I meant the polymorphic nature of Base. – Trevir Oct 18 '16 at 13:14
  • @Trevir Sure, but that doesn’t change my answer. — That said, check the edit I made. – Konrad Rudolph Oct 18 '16 at 13:17
  • Why is the static_cast superior to the dynamic_cast? After all I can check if the dynamic_cast succeeded. – Trevir Oct 18 '16 at 13:23
  • @Trevir But your code isn’t doing that. Using `dynamic_cast` signals “hey, this cast may fail so the user must check the result”. `static_cast` signals “this cast *will* succeed, and I have ensured this with information outside the type system”. It is therefore appropriate to signal to the type system (and maintainers of the code) that the cast’s correctness has been insured elsewhere. – Konrad Rudolph Oct 18 '16 at 13:30