billyquith / ponder

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

classByType<std::string> throws classNotFound #107

Closed shierei closed 5 years ago

shierei commented 5 years ago

My code tries to declare a class which has a std::string property.

Class::declare<Project>("projects") 
  .constructor<>()   
  .property("name", &Project::getname, &Project::setname) ;

The code compiled but it threw classNotFound exception on classByType() at runtime for some reason.

#0  0x00007ffff1c32c1d in __cxa_throw () from /lib64/libstdc++.so.6
#1  0x00007ffff290ad9f in ponder::detail::ClassManager::getById (
    this=0x7ffff2b43900 <ponder::detail::ClassManager::instance()::cm>, id=...)
    at /scratch/backup/ponder/src/classmanager.cpp:99
#2  0x00000000005258cc in ponder::classByType<std::string> ()
    at include/ponder/classget.inl:57
#3  0x0000000000742220 in ponder::detail::UserPropertyImpl<ponder::detail::Accessor2<Project, ponder::detail::FunctionTraits<std::string (Project::*)() const, void> > >::UserPropertyImpl (this=0x7fffc5dbcb00, name=..., accessor=...)
    at include/ponder/detail/userpropertyimpl.inl:64
#4  0x0000000000741f27 in ponder::detail::PropertyFactory2<Project, std::string (Project::*)() const, void (Project::*)(std::string const&), void>::create (
    name=..., accessor1=
    (void (Project::*)(std::string *, const Project *)) 0x59eefc <Project::getname() const>, accessor2=
    (void (Project::*)(Project *, const std::string &)) 0x59ef30 <Project::setname(std::string const&)>)
    at include/ponder/detail/propertyfactory.hpp:392
#5  0x000000000071d94b in ponder::ClassBuilder<Project>::property<std::string (Project::*)() const, void (Project::*)(std::string const&)> (
    this=0x7fff8a7dc4c0, name=..., accessor1=
    (void (Project::*)(std::string *, const Project *)) 0x59eefc <Project::getname() const>, accessor2=
    (void (Project::*)(Project *, const std::string &)) 0x59ef30 <Project::setname(std::string const&)>)
    include/ponder/classbuilder.inl:106

(gdb) fr 2
#2  0x00000000005258cc in ponder::classByType<std::string> ()
    at /ade/shhuang_exac/oracle/oss/thirdparty/ponder/include/ponder/classget.inl:57
57          return detail::ClassManager::instance().getById(detail::typeId<T>());

(gdb) fr 1
#1  0x00007ffff290ad9f in ponder::detail::ClassManager::getById (
    this=0x7ffff2b43900 <ponder::detail::ClassManager::instance()::cm>, id=...)
    at /scratch/backup/ponder/src/classmanager.cpp:99
99              PONDER_ERROR(ClassNotFound(id));

Does anyone know what was going on here? The call was called by ponder implicitly. I added the following line in the example program simple.cpp and it threw the same runtime exception.

const ponder::Class& metaclass2 = ponder::classByType<std::string>();

simple.cpp's Person class also has a std::string property but it runs fine. What have I missed?

Thanks,

shierei commented 5 years ago

I am able to reproduce this by modifying simple.cpp a bit. The argument type of a function cannot be a pointer to std::string. For example, suppose I change the hasBigFeet function in ponder/test/examples/simple.cpp as follows.

bool hasBigFeet(std::string* dummy) const { return shoeSize > 10; }

Then compiling simple.cpp would have the following error.

[ 91%] Building CXX object test/examples/CMakeFiles/egtest.dir/simple.cpp.o
In file included from include/ponder/classget.hpp:38:0,
                 from include/ponder/classbuilder.hpp:35,
                 from ponder/test/examples/simple.cpp:33:
include/ponder/detail/typeid.hpp: In instantiation of  static const char* ponder::detail::StaticTypeId<T>::get(bool) [with T = std::basic_string<char>] :
include/ponder/class.inl:56:60:   required from  static ponder::ClassBuilder<T> ponder::Class::declare(ponder::IdRef) [with T = std::basic_string<char>; ponder::IdRef = ponder::detail::basic_string_view<char>] 
ponder/test/examples/simple.cpp:67:47:   required from here
include/ponder/detail/typeid.hpp:52:46: error:  PONDER_TYPE_NOT_REGISTERED  is not a member of  std::basic_string<char> 
         return T::PONDER_TYPE_NOT_REGISTERED();
                                              ^
make[2]: *** [test/examples/CMakeFiles/egtest.dir/simple.cpp.o] Error 1
make[1]: *** [test/examples/CMakeFiles/egtest.dir/all] Error 2
make: *** [all] Error 2

This compilation error can be avoid by declaring PONDER_TYPE(std::string).

However running egtest with this code would throw the classNotFound exception on class type std::string.

$ egtest
------------------------------------------------------------

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egtest is a Catch v2.2.2 host application.
Run with -? for options

-------------------------------------------------------------------------------
simple tests
  intro
-------------------------------------------------------------------------------
ponder/test/examples/simple.cpp:117
...............................................................................

/scratch/backup/ponder/test/examples/simple.cpp:117: FAILED:
**due to unexpected exception with message:
  the metaclass std::string couldn't be found**

Class Vec2D: 2D vector
    add: add vector to this and return result
===============================================================================
test cases: 3 | 2 passed | 1 failed
assertions: 1 | 1 failed

Not that this is the only way to do pass by reference for a string. As reported in a one of the previous issues, using string& would not work because it acts like pass by value in both CAMP and PONDER.

I am trying to port a project which used CAMP to PONDER. Because of this, I am basically stuck.

billyquith commented 5 years ago

Ok, I'll have some time this weekend. I'll take a look. 👍

shierei commented 5 years ago

Thanks, Billy. Another note. In CAMP, to get rid of this runtime error, I had to also add the following line.

Class::declare("String");

But this did not work for PONDER.

Basically I need a metaclass function to be able to do call by reference, either by string& or string*. Otherwise the function defined becomes useless for me.

billyquith commented 5 years ago

Which platform, compiler, and versions please. I am currently unable to reproduce this.

shierei commented 5 years ago

Red Hat Linux g++ 4.8.5.

billyquith commented 5 years ago

Ok, on the CI tests, gcc 4.8.4 passes: https://travis-ci.org/billyquith/ponder/jobs/456345131

billyquith commented 5 years ago

Are you able to test on a more recent version of the compiler? 4.8.5 is from 2015. I have experienced bugs with GCC before.

shierei commented 5 years ago

Sure. I will give it a try. Thanks

shierei commented 5 years ago

One question. Is using ninja to build ponder absolutely required? I had a hard time installing ninja and dnf on my system. I simply used "cmake . && make" to make ponder and it completed with no error.

billyquith commented 5 years ago

No you can use any build system. I just used Ninja for the example.

shierei commented 5 years ago

I reproduced this on OSX (Mac) using ninja to build as well. My changes in test/examples/simple.cpp are as follows:

37,38d36
< PONDER_TYPE(std::string)
<
57c55
<   bool hasBigFeet(std::string* dummy) const { return shoeSize > 10; } // U.K.!
---
>     bool hasBigFeet() const { return shoeSize > 10; } // U.K.!
69,70d66
<     ponder::Class::declare<std::string>("String");
<
103,104c99
<     std::string dummy = "dummy";
<     const bool bigFeet = ponder::runtime::call(func, person, &dummy).to<bool>();
---
>     const bool bigFeet = ponder::runtime::call(func, person).to<bool>();

Platform and compiler are OSX Apple LLVM version 9.1.0 (clang-902.0.39.2).

shierei commented 5 years ago

simple.cpp.txt

shierei commented 5 years ago

I attached my modified complete simple.cpp above. Please help to see if it reproduces in your system.

Thanks,

billyquith commented 5 years ago

Why are you trying to register std::string? Do you need to do this? It is a basic type, handled by ponder::Value. You don't need to register it to use it. I'm just trying to understand why you need to do this.

shierei commented 5 years ago

As I mentioned earlier, if I did not register it, the code would not compile. If the function's argument type is the basic pass by value type std::string, I did not have to register. For the pointer type std::string*, I had to for some reason. For the reference type std::string&, it was a whole different issue and the registration did not help either.

shierei commented 5 years ago

I think I figure out where the issue is. It comes down to the difference in declaring a metaclass between CAMP and PONDER. In CAMP, it is defined as follows.

static ClassBuilder declare(const std::string& name)

A metaclass declared can be given an arbitrary name as long as it is unique.

Ponder declares it as follows.

static ClassBuilder ponder::Class::declare (IdRef id = ponder::Id())

It does not allow a metaclass declared to be given an arbitrary name. Based on the implementation of detail::ClassManager::addClass(), the name given should be either empty or be the same as the C++ class typename.

That is, if I declare the metaclass std::string as

ponder::Class::declare("");

or

ponder::Class::declare("std::string");

The simple.cpp code I attached above would run fine.

shierei commented 5 years ago

It ran fine after I registered and metaclass declared std::string. But after that the ponder::Value::kind() for std::string return type ponder::User instead of ponder::String. Do you know why and a workaround?

Thanks,

shierei commented 5 years ago

I wonder if you have reproduced this problem and know a solution for it?

billyquith commented 5 years ago

Sorry, I was on holiday. ⛅️

I did have a look and I think the problem is that ponder::Value does not handle referencing types for the value types that it holds (e.g. pointers). I haven't checked whether CAMP did. This may be something that got lost in the refactoring. I did look into fixing this, but I didn't come up with a satisfactory solution yet. I'll try and have another look this week. This should be something you should be able to do.

Your workaround is to register std::string as a user type so that the reference is dealt with as a UserData. This has different implementations for reference and value semantics, whereas Value, for non-UserData types, does not.

std::string s;
foo(&s); // void foo(std::string*);

In the above call, at the point of setting the argument, we can dereference s, but when we want to set the value, we have lost the reference. This is the problem that needs solving.

shierei commented 5 years ago

Welcome back and thanks for your help as always. For CAMP, if I need to use any reference simple type, such as int or string, as a function argument, I also need to register the simple type. However CAMP is able to preserve the simple type in Value. The simple type after registration is lost in Ponder. It becomes the User type and as a result I am not able to convert it back to the simple type. The conversion would throw an exception, similar to the following:

value of type user couldn't be converted to type int

shierei commented 5 years ago

I want to add that once I registered the simple type, any property getter which returns this simple type becomes a User type. This seems to already happen at the class declaration time when the property is added to the class builder. If I do not register the simple type, then the property getter returning type is correct. But I had to do the registration to get the code compiled and run. With a User type, I won't be able to retrieve the property value back to the simple type. This is the dilemma that I am facing.

shierei commented 5 years ago

I spent some time to track down the reason for this behavior to the following AccessTraits definition in propertyfactory.hpp.

template <typename T>
struct AccessTraits<T,
    typename std::enable_if<StaticTypeId<T>::defined && !std::is_enum<T>::value>::type>
{
    static constexpr PropertyAccessKind kind = PropertyAccessKind::User;
 ...

This says if a type, including any basic type, is registered as a ponder type, it is automatically considered as a user defined type. I guess we could exclude some C++ basic types in the Boolean expression of enable_if().

billyquith commented 5 years ago

I want to add that once I registered the simple type, any property getter which returns this simple type becomes a User type.

This says if a type, including any basic type, is registered as a ponder type, it is automatically considered as a user defined type.

I think I would rather not allow basic types (handled by Value) to be registered as UserTypes. If Values could also be referenced then this would not be necessary. Currently you can only return parameter values by reference for UserTypes, e.g. void foo(Type*) and not void foo(int*), but it would be nice to be able to do so.

The change you are suggesting would allow basic types to be passed as UserTypes, but I think there would be complications where this turns out to be ambiguous. One solution is to add pointer versions of the basic types to value (assuming that they are all references, and not owned).

shierei commented 5 years ago

I agree . The best fix is to allow to use pointer to a basic type without having to register that basic type as a User type. I am not sure if defining ValueMapper<T*> would make that happen. But I am happy to find a solution by modifying the AccessTraits. Note that CAMP works exactly like this. To use a pointer type to a basic type, you need to register that basic type and declare it as a metaclass.

shierei commented 5 years ago

I created two new issues which summarize the discussions in this thread.

billyquith commented 5 years ago

I'll close this issue as we created two other issues. V3.1 is now released which fixes one of those issues. The other issue is covered in #109.