Function returning dynamicly casted pointer, return pointer as if it wasn't casted at all

Question

I have a base class of Component and mulitple deriving classes like MeshComponent. The Entity class stores all Components as std::shared_ptr in std::unordererd_map caaled m_Components, so I create the following function to get component from the map:

const std::shared_ptr<Component>& Entity::GetComponent(ComponentType type)
{   
    switch (type)
    {
        case ComponentType::None:
            EG_CORE_ASSERT(false, "Component of type None is not supported!");
            return nullptr;
        case ComponentType::Transform:
            return std::dynamic_pointer_cast<TransformComponent>(m_Components[ComponentType::Transform]);
        case ComponentType::Mesh:
            return std::dynamic_pointer_cast<MeshComponent>(m_Components[ComponentType::Mesh]);
    }
}

ComponentType is an enum class that hold all the components' types. The problem is that when I call this function in my main file returned pointer I just pointer to base class and I cannot call function specific for certain component.

The thing that I tried is casting these pointers in my main files like so: std::dynamic_pointer_cast<Engine::MeshComponent>(testEntity.GetComponent(Engine::ComponentType::Mesh))->SetVertexArray(m_VertexArray);

But this return me memrory acces violation.

Christophe · Accepted Answer · 2019-11-09T17:19:19.993

0

What's wrong with your code ?

Your function will return a shared pointer to the base class. It's the return type that you have defined, so you need to downcast like you did.

But you made something wrong here: you return a reference, and this reference will be to a temporary shared pointer that will be destroyed as soon as you return from GetComponent() causing UB.

You need to redefine your function and use a value return type, be removing the & behind const std::shared_ptr<Component>:

const std::shared_ptr<Component> Entity::GetComponent(ComponentType type)

This should solve your problem if you did the downcast to the correct type.

what could be improved in your code ?

But you should accept that the downcasting can go wrong. The whole purpose of dynamic_pointer_cast is to allow this kind of safety check. So instead of:

std::dynamic_pointer_cast<Engine::MeshComponent>(testEntity.GetComponent(Engine::ComponentType::Mesh))->SetVertexArray(m_VertexArray);

do it in two steps:

auto pm = std::dynamic_pointer_cast<Engine::MeshComponent>(testEntity.GetComponent(Engine::ComponentType::Mesh)); 
if (pm) 
    pm->SetVertexArray(m_VertexArray);
else std::cout << "Oh oh ! Something went wrong"<<std::endl;

Here an online demo on a minimalist example. You can play with it online, and experiment: adding the & to the return type will cause a runtime error here.

Polymorphism shoud not require casting

If you have poymorphic code and have to do a lot of casting, then there is something wrong with the design.

First, there is no way to make a polymophic Getcomponent() that would allow you to immediately call a function that does not exist for the polymorphic type:

Here you do a dynamic casting in the function, but since the return type of the function is compile time, your dynalic casting immediately gets reverted. This is why you need to a (risky) downcasting with the pointer returned.
There is no way of doing it with templates. Because templates are also based on the compile time types.

What you could do is to define three different functions, each returning the expected type. But then you have to take extra care because getting a pointer this way, not being sure that a pointer is found in the first place, and not being sure that the dynamic cast will succeed, might result in the rdereferencing of a nullptr which is UB and could crash your software.

Recommendation:

Try to design the member functions of Component as polymorphic as possible, so that the user des not have to know if it's mesh or not when invoking a function. Downcasting should be the exception.

If it is not possible, and if you'd still prefer to go for the specialised functions (e.g. GetMeshComponent()) (thus making your design much less extensible), then you should foresee some exception handling in case this specialised function is not able to provide the expected pointer.

edited Nov 09 '19 at 17:19

answered Nov 09 '19 at 15:57

Christophe

68,716
7
72
138

So how do I define the right return type so that I can use `GetComponent()` function? And this approach with 2 steps give me a memory acces violation as well – Szahu Nov 09 '19 at 16:03
@Szahu as I showed: you need to remove the & after the return type ! – Christophe Nov 09 '19 at 16:05
Well, but Can I make it work like this: `testEntity.GetComponent(Engine::ComponentType::Mesh)->//Function` instead of casting. I want the `GetComponent()` function to take care of all the casting. And removing '&' didnt work for me – Szahu Nov 09 '19 at 16:13
@Szahu: Take care of all *what* casting? You're not casting anything right now. Or rather, you're casting and then undoing it. Are you trying to make a function that returns different types based on a parameter? Because that's not allowed. – Nicol Bolas Nov 09 '19 at 16:19
@Szahu the hole point is that the return type of a function is something that is defined at compile time. If you return a shared pointer to component, you cannot use it directly as if it were a mesh component. In your GetCoponent() you lade some castings, but these are ilmediately recasted to match the return type. THe question is if you cannot make Component more polymorphic, so that the user do not need to know which kind of component it is. This would me much more convenient and could get rid of all these casting issues. – Christophe Nov 09 '19 at 16:27
@NicolBolas Please forgive any missunderstans, To clarify: what Im trying to do is a function that would allow me to to something like this: `myEntity.GetComponent()->somefunction()` but the function I created returned just base class. So I tried casting the returned pointer again. It didnt give me `Component has no member ...` Errpr but it gave me memory acces violation error – Szahu Nov 09 '19 at 16:30
@Szahu: What is `somefunction` in this example? Is it not a part of the base class? – Nicol Bolas Nov 09 '19 at 16:31
Yes, it is only part of the deriving class. If it was the part of base class there wouldnt be a problem. I could maybe declare all the functions in the base class and then override them but this is super ugly solution – Szahu Nov 09 '19 at 16:33
@NicolBolas can I maybe solve this using templates somehow? – Szahu Nov 09 '19 at 16:46
@Szahu the whole point is that this that it is not possible to do whant you want to do the way you are trying to do it. You can of course use templates and hope for a nice type deduction. But template are compile-time and not runtime. – Christophe Nov 09 '19 at 17:03
@NicolBolas guees Ill have to stick to your 2 step method, and figure out how to get rid of that mempry error. Thanks for help! – Szahu Nov 09 '19 at 17:16
@Szahu I've edited the answer to address the polylorphism issues that we discussed in the comments. – Christophe Nov 09 '19 at 17:21

Function returning dynamicly casted pointer, return pointer as if it wasn't casted at all

1 Answers1

What's wrong with your code ?

what could be improved in your code ?

Polymorphism shoud not require casting