billyquith / ponder

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

Having problems to get the property value of a user defined type #110

Closed shierei closed 5 years ago

shierei commented 5 years ago

Suppose I define a object type as follows.

typedef struct myObj
{
  int a;
  int b;
} myObj;
PONDER_TYPE(myObj)

And use it for a property type of the Person class in simple.cpp.

class Person
{
public:
  myObj getObj() const { return m_obj; }
  void setObj(const myObj& obj) { m_obj = obj; }
...
private:
  myObj m_obj;
...
}

Register it.

   ponder::Class::declare<myObj>("myObj");
   ponder::Class::declare<Person>("Person")
        .constructor<std::string>()
        .property("obj", &Person::getObj, &Person::setObj)
...

Testing it as below.

    myObj obj;
    obj.a = 2;
    obj.b = 5;
    person.set("obj", obj);
    myObj retObj = person.get("obj").to<myObj>();
    std::cout << "a = " << retObj.a << " b = " << retObj.b << std::endl;

I expected retObj = obj but the output is: a = -10368 b = 32767

shierei commented 5 years ago

This seems to be related to creating a UserObject using a reference or a copy of the C++ object. Currently it uses a reference and the referenced object apparently was freed or out of scope after the member function returns. I tried the fix below and that seemed to have corrected the problem.

In userpropertyimpl.inl, there is the following code:

template <typename A>
Value UserPropertyImpl<A>::getValue(const UserObject& object) const
{
    return ToUserObject<A::canWrite>::get(
                m_accessor.m_interface.getter(object.get<typename A::ClassType>()));
}

I changed it to

template <typename A>
Value UserPropertyImpl<A>::getValue(const UserObject& object) const
{
    return ToUserObject<A::RefTraits::isRef>::get(  
                m_accessor.m_interface.getter(object.get<typename A::ClassType>()));
}
billyquith commented 5 years ago

I had a bit of time to look at this this evening. - I merged your change in, but I think my code was right in the first place. The spurious getObject() function was confusing (and now removed).

I put in some extra tests in the UserProperty tests which I think mirror your behaviour above and they seem to work ok.

Would you mind branching off the develop branch if you have future fixes please. I'm sort of following Github workflow. I should put some notes up. 🎶

shierei commented 5 years ago

I used my forked master branch to submit my first pull request which completed the comparison operators for ponder::Value. That was a mistake because I accidentally push my working branch to the master branch. However, the pull request for this fix supposedly was directly from my dev branch instead of from the master branch. Did you still see it not following the Github workflow?

shierei commented 5 years ago

Unfortunately your recent fix on top of my fix broke it again. See the attached modified simple.cpp to illustrate the bug introduced.

simple.cpp.txt

billyquith commented 5 years ago

Unfortunately your recent fix on top of my fix broke it again. See the attached modified simple.cpp to illustrate the bug introduced.

I needed to fix for return by value. There should be unit tests for all cases now.

Thanks for your patience. Tiring doing this and a day job! I'll push out 3.1.

billyquith commented 5 years ago

I used my forked master branch to submit my first pull request which completed the comparison operators for ponder::Value. That was a mistake because I accidentally push my working branch to the master branch.

Yep that was it. If people fix master then I end up working in master and ideally only merges from the release branch should go into master. Then you have clear release points. Otherwise it's a moving target. - I suppose one way to fix it is to make develop the exposed branch in GH and then point people to master who want a release point.