Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

classByType<std::string> throws classNotFound #107

Closed
shierei opened this issue Nov 14, 2018 · 26 comments
Closed

classByType<std::string> throws classNotFound #107

shierei opened this issue Nov 14, 2018 · 26 comments
Assignees

Comments

@shierei
Copy link
Contributor

shierei commented Nov 14, 2018

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 classByTypestd::string() 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 shierei changed the title classByType<std::string> throw classNotFound classByType<std::string> throws classNotFound Nov 14, 2018
@shierei
Copy link
Contributor Author

shierei commented Nov 16, 2018

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
Copy link
Owner

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

@billyquith billyquith self-assigned this Nov 16, 2018
@shierei
Copy link
Contributor Author

shierei commented Nov 16, 2018

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

Class::declarestd::string("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
Copy link
Owner

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

@shierei
Copy link
Contributor Author

shierei commented Nov 17, 2018

Red Hat Linux g++ 4.8.5.

@billyquith
Copy link
Owner

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

@billyquith
Copy link
Owner

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
Copy link
Contributor Author

shierei commented Nov 17, 2018

Sure. I will give it a try. Thanks

@shierei
Copy link
Contributor Author

shierei commented Nov 17, 2018

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
Copy link
Owner

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

@shierei
Copy link
Contributor Author

shierei commented Nov 17, 2018

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
Copy link
Contributor Author

shierei commented Nov 17, 2018

simple.cpp.txt

@shierei
Copy link
Contributor Author

shierei commented Nov 17, 2018

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

Thanks,

@billyquith
Copy link
Owner

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
Copy link
Contributor Author

shierei commented Nov 19, 2018

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
Copy link
Contributor Author

shierei commented Nov 20, 2018

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::declarestd::string("");

or

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

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

@shierei
Copy link
Contributor Author

shierei commented Nov 21, 2018

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
Copy link
Contributor Author

shierei commented Nov 27, 2018

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

@billyquith
Copy link
Owner

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
Copy link
Contributor Author

shierei commented Dec 3, 2018

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
Copy link
Contributor Author

shierei commented Dec 4, 2018

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
Copy link
Contributor Author

shierei commented Dec 5, 2018

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
Copy link
Owner

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
Copy link
Contributor Author

shierei commented Dec 5, 2018

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
Copy link
Contributor Author

shierei commented Dec 5, 2018

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

@billyquith
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants