Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Polymorphic assignment of raw pointer properties does not work #56

Closed
rovarma opened this issue Apr 26, 2017 · 2 comments
Closed

Polymorphic assignment of raw pointer properties does not work #56

rovarma opened this issue Apr 26, 2017 · 2 comments

Comments

@rovarma
Copy link

rovarma commented Apr 26, 2017

I guess this is more a bug + feature request rolled into one.

First, the RTTR equivalent of the following regular C++ code does not work (don't have a working sample right now, but I think you get it):

class Base
{
};

class Derived : public Base
{
};

class Foo
{
	Base* mBase;
};

Derived* derived = new Derived();
Foo foo;
foo.mBase = derived; // Assigning pointer-to-derived to pointer-to-base. Works in regular C++, fails in RTTR.

So, we're new'ing a Derived, then assigning that to a pointer-to-base through RTTR.

In addition, the following situation does not work in either C++ or RTTR, but can work in RTTR and is extremely useful for things like object deserialization, so consider this a feature request:

class Base
{
};

class Derived : public Base
{
};

class Foo
{
	Derived* mDerived;
};

Base* base= new Derived();
Foo foo;
foo.mDerived = base; // Assigning pointer-to-base to pointer-to-derived. Fails in both C++ and RTTR.

So, we're new'ing a Derived, then assigning that to a pointer-to-base, then trying to assign that to a pointer-to-derived. In C++ this is illegal, because the compiler can't know that Base* is actually a valid Derived* instance (so it requires an explicit cast), but in RTTR we can know that this is a valid assignment, since we know the actual type of the instance.


The first bug (not being able to assign pointer-to-derived to pointer-to-base) is caused by the fact that ptr_type<T> argument::is_type() returns false when faced with this situation. That, in turn, is caused by the implementation to be wrong:

template<typename T>
RTTR_INLINE argument::ptr_type<T> argument::is_type() const RTTR_NOEXCEPT
{
    return ((rttr::type::get<T>() == m_type) ||
             m_type == type::get<std::nullptr_t>() ||
             (m_variant && type::get<variant*>() == type::get<T>()));
}

Specifically it's about the rttr::type::get<T>() == m_type line. This is a specific type match, which in this situation will return false since the type does not actually match exactly. Instead, the code should check if the types match through a is_derived_from check, ie:

template<typename T>
RTTR_INLINE argument::ptr_type<T> argument::is_type() const RTTR_NOEXCEPT
{
    // T is the type of property we're trying to assign *to*, m_type is the type of argument we're trying to assign *from*
    return ((m_type.is_derived_from<T>()) ||
             m_type == type::get<std::nullptr_t>() ||
             (m_variant && type::get<variant*>() == type::get<T>()));
}

This can be further extended to also implement the 'feature request': by not checking m_type directly, but instead using the most-derived-type of the pointer being assigned, we can also assign pointer-to-base to pointer-to-derived:

template<typename T>
RTTR_INLINE argument::ptr_type<T> argument::is_type() const RTTR_NOEXCEPT
{
	// In order to allow assigning a pointer-to-base to a pointer-to-derived (technically illegal in C++, but useful in an RTTI system),
	// we need to retrieve the actual type of the pointer we're assigning.
	type derived_type = m_type;
	if (m_data != nullptr)
	{
		void* ptr = *reinterpret_cast<void**>(const_cast<void *>(m_data));
		if (ptr != nullptr)
			derived_type = type::get_derived_type(ptr, m_type);
	}

	return (derived_type.is_derived_from<T>() ||
		m_type == type::get<std::nullptr_t>() ||
		(m_variant && type::get<variant*>() == type::get<T>()));
}

Now, while I've tested this code and it works for us, I'm less sure about it because I don't know how it behaves in the presence of multiple base classes. In theory it should work fine, but I don't know enough about multiple base classes to say for sure (I barely ever use them).

@acki-m
Copy link
Contributor

acki-m commented May 8, 2017

Hi Rovarma,

That's actually not a bug it is desired behavior.
When you set a property value or invoke a method, the value type has to match exactly the type.
The performance implication are otherwise too big. Because in most cases you will have the correct type.
That's how C# does it and also other reflection engines I researched.

However, what you want to do Is possible with an additional step and even more dynamically then you might think:
You have to use the variant class to convert your pointer type.

auto prop = type::get<Foo>().get_property("derived");

Derived d;
Base* b = &d;

variant var = b;
auto ret = var.convert(prop.get_type()); // will convert internaly 'Base*' to 'Derived*'

Foo obj;
prop.set_value(obj, var); // will succeed

What do you think, can you live with this?

PS: When creating a variant, with a pointer type (i.e. type <=8 bytes), it will don't create any heap allocation, so it it quite fast.
PPS: Do you know about rttr_cast?
PPPS: rttr supports of course multiple inheritance, but what is not supported is, having one base class multiple times in the inheritance hierarchy.

@rovarma
Copy link
Author

rovarma commented May 9, 2017

Hey,

Thanks, I didn't know about convert. That might be fine for us; I'll test it later.
It might be a good idea to document the fact that this is intended behavior somewhere; coming from C++ it's very natural to be able to assign a pointer-to-derived to a pointer-to-base, but I understand the performance concerns.

Yes, I do know about rttr_cast, but that function assumes you know the type you're casting to at compile-time, which is something we don't know.

I'll keep you posted.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants