billyquith / ponder

C++ reflection library with Lua binding, and JSON and XML serialisation.
http://billyquith.github.io/ponder/
Other
640 stars 93 forks source link

Unwanted implicit conversion in constructor marked explicit #44

Closed iwbnwif closed 8 years ago

iwbnwif commented 8 years ago

This might be better on StackOverflow, but as it directly relates to Ponder I thought I would try here first.

I have a class (actually a dialog box) that users Ponder to display an object.

The ctor declaration looks something like this: explicit MyDataObjectDialog (..., const ponder::UserObject& object, ....);

Where a member reference variable m_Object is initialized with object in the ctor's base initializer list.

I was facing a problem when I accidentally called the ctor like this:

// Causes crash in userobject.cpp because m_holder isn't valid?
myObject object;
MyDataObjectDialog dialog (..., object, ...);

... show dialog ...
... access updated object ...

// Works fine
myObject object;
ponder::UserObject uObject (object);
MyDataObjectDialog dialog (..., uObject, ...);

... show dialog ...
... access updated object ...

I believe this is because the ponder::UserObject created in the first call is temporary and goes out of scope once the ctor is finished. This is all quite understandable.

My question is why myObject is [implicitly] being accepted by the explicit ctor that is expecting a const ponder::UserObject&? Also if this sort of conversion is valid, shouldn't Ponder be able to handle it?

This is on Linux using 4.8.4.

iwbnwif commented 8 years ago

My question is why myObject is [implicitly] being accepted by the explicit ctor that is expecting a const ponder::UserObject&?

Oops, of course it is because the template <typename T> UserObject(const T& object); is being used.

billyquith commented 8 years ago

Yes, that is correct. You should also be sure which type of user object implementation is being used. I.e. UserObject is a wrapper for both references to objects and instances. To be sure which one you are using you can use the ref() and copy() (docs)

iwbnwif commented 8 years ago

Thank you for the reply.

I did see that in the docs, what surprised me (although it shouldn't) was the implicit conversion using the const T& object ctor. In some ways it would be nice for this ctor to be explicit to avoid this kind of mistake.

The other surprise (which again shouldn't be one) is that the referenced object went out of scope without Ponder complaining. However having searched, it seems there is no way for Ponder to tell whether the original object reference by a UserObjectstill exists :(.

So I will close this now, unless there is some magic that could be done using weak_ptr to throw an exception rather than cause a segfault.

billyquith commented 8 years ago

TBH, that's a fairly standard scoping bug. I'd just call that a bug, learn from it and move on.

Although, I've recently thought about adding policies to UserObjects. Currently when you create a UO it has the following members:

    /// Metaclass of the stored object
    const Class* m_class;

    /// Optional abstract holder storing the object
    std::shared_ptr<detail::AbstractObjectHolder> m_holder;

    /// Optional parent object
    std::unique_ptr<ParentObject> m_parent;

    /// Optional pointer to the child object (m_parent.object.m_child == this)
    const UserObject* m_child;

I don't have any code examples for uses of the CAMP, the original library, so I'm not 100% sure how, or how often, these features are used. Looks like some memory could be reclaimed and things simplified. Also, you might want to avoid multiple allocations for a reference (i.e. an abstract implementation of holder is also allocated).

Perhaps policies could be added that allow compile-time decisions about how objects are referenced and stored, with UO still being the abstraction.

iwbnwif commented 8 years ago

Perhaps policies could be added that allow compile-time decisions about how objects are referenced and stored, with UO still being the abstraction.

This sounds like an excellent solution. Personally, I prefer to avoid implicit conversion in most cases so if there was a compile time option to prevent that, it would get my vote!