Raw pointer properties don't seem to work

[rovarma]

I'm trying to register a class with raw pointer properties (i.e. no smart pointer stuff) and RTTR (0.9.5) does not seem to handle it correctly. The issue seems to be that pointers are being assigned by value. This manifests itself in two different ways:

1. Setting a value on a pointer property whose current value is null will sometimes crash, but when it doesn't crash won't assign anything at all.
2. A pointer property of a type whose assignment operator is inaccessible will not compile at all.

Both are pretty unexpected, but looking at the code this seems to be by design, which seems pretty odd. I'm building on Windows using VS2015, for x64 bit.

Minimal repro case:

struct Pointee
{
public:
	// If you uncomment this line the code will no longer compile. If you leave it commented, the code will compile but crash
	//Pointee& operator=(const Pointee&) = delete;
};

struct ClassWithPointer
{
public:
	Pointee* mPointee = nullptr;	
};


RTTR_REGISTRATION
{
	using namespace rttr;

	registration::class_<Pointee>("Pointee")
					.constructor<>();

	registration::class_<ClassWithPointer>("ClassWithPointer")
					.constructor<>()(policy::ctor::as_raw_ptr)
					.property("mPointee", &ClassWithPointer::mPointee);
}


// Main loop
int main(int argc, char *argv[])
{
	rttr::type obj_type = rttr::type::get<ClassWithPointer>();
	ClassWithPointer* obj = obj_type.create().get_value<ClassWithPointer*>();

        // This line will either crash, or if it doesn't crash, the mPointee member will still be null
	obj_type.get_property("mPointee").set_value(*obj, new Pointee());
}

Am I doing something obviously wrong?

[rovarma]

I've done a test using the latest version of RTTR (master) and that has the same issue.

[acki-m]

I found the reason for this issue, but I have to think about how to fix it now easily.
btw. You should not use raw pointers 😉

[rovarma]

I've investigated a bit more and I've found a fix. I'm not sure if it's okay with you, but I'll make it locally so I can continue.

So, the problem seems to be that there's two uses of pointers in RTTR:

1. Where you're binding a property by pointer, through the policy::prop::bind_as_ptr policy
2. When you're actually binding a raw pointer property

In the first case, we don't actually want to update the pointer itself, rather we want to update the value of thing the pointer points to, which explains the current copy behavior when setting pointers. In this case, the pointer should logically be considered a const pointer to a non-const object, even though it's not.

In the second case, we do want to update the pointer itself; the pointer is the value.

So, the fix I've made is as follows. First, I've modified the current specialization of property_accessor<T*> to consider the pointer const:

template<typename T>
struct property_accessor<T const*>
{
    static bool set_value(T* prop, argument& arg)
    {
        *prop = *arg.get_value<T*>();
        return true;
    }
};

This allows us to select the correct specialization to use, depending on the desired behaviour.

Next, I've updated property_wrapper for the return_as_ptr, set_as_ptr specializations, in order to maintain the current behavior in case 1:

property_wrapper_member_object.h line 161 becomes:

return property_accessor<A const*>::set_value(&(ptr->*m_acc), arg);

and property_wrapper_object.h line 151 becomes:

return property_accessor<C const*>::set_value(m_accessor, arg);

(note the addition of the const in the template selection)

This change allows the 'regular' specialization of property_wrapper to select use the non-specialized version of property_accessor, which has the correct behavior for raw pointers.

This seems to work for me and all unit tests still pass. Do you think this could be a good way to fix it?

I'll use raw pointers if I want to 😎 (in this case it's for some high performance stuff).

[rovarma]

In working with pointers more, we've noticed that support for pointers just seems broken in general. At the very least, polymorphic pointer assignment (assign pointer-to-derived-type to pointer-of-base-type) and pointer comparison don't work. There may be other issues with pointers we just haven't run into yet.

We have local fixes for both of them. Should I open new bugs for these things or is it OK for this bug to serve as an assortment of pointer problems?

[acki-m]

@rovarma
Sorry for delay I was in vacation.
I am impressed how good you found yourself in the code base. 👍
Do you want to become a developer for RTTR?

The following specialization is made to be used only for the property policy bind_as_ptr
But it is also used for normal pointers, which is wrong.

template<typename T>
struct property_accessor<T*>
{
    static bool set_value(T* prop, argument& arg)
    {
        *prop = *arg.get_value<T*>();
        return true;
    }
};

Your specialization with the const Template argument, should not be necessary,
Just the normal implementation should be called:

template<typename T>
struct property_accessor
{
    static bool set_value(T& prop, argument& arg)
    {
        prop = arg.get_value<T>();
        return true;
    }
};

So I need to make the specialization only be called with the policy.
That's why I wonder, how your adding of const expression make it working.

Yes, please report any bugs you found and when you found one, open up a new issue for every bug.

[rovarma]

No problem. Hope you had a good holiday.

The reason why the specialization is called even without the policy is because in the regular property_wrapper, in the case of a pointer, typename A will be of type Type*. So the compiler selects the T* specialization of property_accessor, because it is a better (more specialized) match than the normal, unspecialized, template.

Changing the specialization from T* to T const* fixes it, because now the normal template is a better match, since the type being used to call it is of type T*, not of T const*, and calling the T const* specialization would therefore require type conversion. If we're fixing it in this way, we then need to update the bind_as_ptr policy cases to use const so that they still use the T* specialization.

Of course, if the intent is really that the T* specialization is only called in the policy case, another way to fix it could be to remove the specialization entirely and simply introduce a new property_accessor type that is called instead (or somehow specialize on policy?).

I'll leave it up to you to decide which fix you prefer :)

EDIT: I've opened bugs for the other two issues we've encountered: #55 #56