billyquith / ponder

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

call by reference, using a pointer or a non-const reference, does not work #109

Open shierei opened 5 years ago

shierei commented 5 years ago

Declaring a metaclass function with a pointer or a non-const reference to any basic type as an argument does not compile. The pointer to a basic type can be made compilable if you register this basic type. But if you register this basic type, and property or function returning value of this type becomes ponder::ValueKind::User. This prevents you from converting the property value or the function return value back to the basic type. The conversion would throw an exception. It basically makes this basic value type used as a property value or a function returning value not visible to the client.

billyquith commented 5 years ago

This is an awkward one to fix. However, I would like to fix it. It is tricky because Values handle basic types, but they do not support references to basic types. Hence you have to reference them as user types, which do support references (both ref and values). It seems a bit overkill to support basic types like this, but perhaps that should be a default solution until I can think of a better one.

billyquith commented 5 years ago

Note to self: Value holder policies. external/internal.

shierei commented 5 years ago

I have a fix to support pointer typed argument nicely. That is all you need if you want a metaclass's function to return something through a function argument with a pointer to a basic type.

billyquith commented 5 years ago

Sure, yes, I’d be interested in seeing it.

shierei commented 5 years ago

I like to show you in a tested code but it looks like currently the ponder branch is not buildable.

billyquith commented 5 years ago

What seems to be broken? All of the tests appear to be passes in develop and master branches on all platforms.

shierei commented 5 years ago

I was not able to build on a RedHat platform. On Mac, it built however. Below is the build error.

[  1%] Building CXX object CMakeFiles/ponder.dir/src/class.cpp.o
In file included from ./ponder/include/ponder/detail/idtraits.hpp:57:0,
                 from ./ponder/include/ponder/config.hpp:79,
                 from ./ponder/include/ponder/classget.hpp:36,
                 from ./ponder/include/ponder/class.hpp:34,
                 from ./ponder/src/class.cpp:30:
./ponder/include/ponder/detail/string_view.hpp: In instantiation of  constexpr int ponder::detail::basic_string_view<CharT, Traits>::compare(ponder::detail::basic_string_view<CharT, Traits>) const [with CharT = char; Traits = std::char_traits<char>] :
./ponder/include/ponder/detail/string_view.hpp:301:85:   required from  constexpr bool ponder::detail::basic_string_view<CharT, Traits>::operator<(const ponder::detail::basic_string_view<CharT, Traits>&) const [with CharT = char; Traits = std::char_traits<char>] 
./ponder/include/ponder/detail/dictionary.hpp:46:49:   required from  bool ponder::detail::DictKeyCmp<T>::operator()(T, T) const [with T = ponder::detail::basic_string_view<char>] 
./ponder/include/ponder/detail/dictionary.hpp:102:59:   required from  ponder::detail::Dictionary<KEY, KEY_REF, VALUE, CMP>::const_iterator ponder::detail::Dictionary<KEY, KEY_REF, VALUE, CMP>::findKey(KEY_REF) const [with KEY = std::basic_string<char>; KEY_REF = ponder::detail::basic_string_view<char>; VALUE = std::shared_ptr<ponder::Function>; CMP = ponder::detail::DictKeyCmp<ponder::detail::basic_string_view<char> >; ponder::detail::Dictionary<KEY, KEY_REF, VALUE, CMP>::const_iterator = __gnu_cxx::__normal_iterator<const ponder::detail::Dictionary<std::basic_string<char>, ponder::detail::basic_string_view<char>, std::shared_ptr<ponder::Function> >::pair_t*, std::vector<ponder::detail::Dictionary<std::basic_string<char>, ponder::detail::basic_string_view<char>, std::shared_ptr<ponder::Function> >::pair_t, std::allocator<ponder::detail::Dictionary<std::basic_string<char>, ponder::detail::basic_string_view<char>, std::shared_ptr<ponder::Function> >::pair_t> > >] 
./ponder/include/ponder/detail/dictionary.hpp:119:40:   required from  bool ponder::detail::Dictionary<KEY, KEY_REF, VALUE, CMP>::tryFind(KEY_REF, ponder::detail::Dictionary<KEY, KEY_REF, VALUE, CMP>::const_iterator&) const [with KEY = std::basic_string<char>; KEY_REF = ponder::detail::basic_string_view<char>; VALUE = std::shared_ptr<ponder::Function>; CMP = ponder::detail::DictKeyCmp<ponder::detail::basic_string_view<char> >; ponder::detail::Dictionary<KEY, KEY_REF, VALUE, CMP>::const_iterator = __gnu_cxx::__normal_iterator<const ponder::detail::Dictionary<std::basic_string<char>, ponder::detail::basic_string_view<char>, std::shared_ptr<ponder::Function> >::pair_t*, std::vector<ponder::detail::Dictionary<std::basic_string<char>, ponder::detail::basic_string_view<char>, std::shared_ptr<ponder::Function> >::pair_t, std::allocator<ponder::detail::Dictionary<std::basic_string<char>, ponder::detail::basic_string_view<char>, std::shared_ptr<ponder::Function> >::pair_t> > >] 
./ponder/include/ponder/class.inl:78:37:   required from here
./ponder/include/ponder/detail/string_view.hpp:201:5: error: body of constexpr function  constexpr int ponder::detail::basic_string_view<CharT, Traits>::compare(ponder::detail::basic_string_view<CharT, Traits>) const [with CharT = char; Traits = std::char_traits<char>]  not a return-statement
     }
     ^
make[2]: *** [CMakeFiles/ponder.dir/src/class.cpp.o] Error 1
make[1]: *** [CMakeFiles/ponder.dir/all] Error 2
make: *** [all] Error 2
billyquith commented 5 years ago

Please note compiler version notes. C++14 required.

shierei commented 5 years ago

Cmake automatically configured it to use std=gnu++1y on the RedHat platform.

shierei commented 5 years ago

This must be new since I was able to build just a few days ago.

billyquith commented 5 years ago

Yes. It’s mentioned in the change log and release notes.

billyquith commented 5 years ago

This must be new since I was able to build just a few days ago.

I put a more explicit note about C++14 support in the notes. Sorry if it wasn't clear.

shierei commented 5 years ago

I upgraded my gcc to version 7.2 and it built fine for me. Please see my pull request for this discussion thread. Thanks.

billyquith commented 5 years ago

Sorry, busy week at work. I have looked at the PR. Hopefully your patch works for now.

I had a think on the way to work. My current thinking is to add calling policies and/or argument policies. This would allow customisation of the argument passing. CAMP/Ponder currently converts arguments to Values and then unpacks them at the other end. This is slow and inefficient, so we could be more direct (avoiding a variant, and just checking the argument is correct or convertible).

billyquith commented 5 years ago

I made some progress on this. There is a "feature/arg" branch with a working pass-by-ref for pointers. I'm not going to push it out because it doesn't handle references, just pointers.

I'm thinking of adding a "non-owned reference" type, which Value would use as a variant type. The basic types convert into their nearest equivalent type, but the reference would have to be identical. This is necessary because non-const references can't be returned otherwise (unless we convert them to UserObject reference holders, which is expensive and requires registering the basic types).

Ideally I'd like a "direct call" mechanism as well as a "Value call" mechanism. Direct call could be used for known compile-time function signatures, and "Value call" could be used where we perhaps have runtime data-driven reflection and we don't know the type of anything until we use it. Direct calling means we could bypass some of the inefficiencies of the temporary Value call. The Lua calling mechanism works like this.

Sorry if you aren't interested in all these ramblings. Just having a brain dump!

billyquith commented 5 years ago

I experimented with adding a ValueRef type that is a cheaper version of UserObject byRef. This would mean you don't have to declare basic types to use them by reference.

shierei commented 5 years ago

That sounds promising. Will call by pointer work also?

billyquith commented 5 years ago

Finally checked this into the develop branch. I'm a bit rusty no what I've done!