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

Trying to wrap a C++ class that is non-copyable will fail to compile, unless a operator<< is defined for the class, too. #22

Closed
cstim opened this issue Jan 8, 2021 · 2 comments · Fixed by #30
Assignees
Labels
bug Something isn't working

Comments

@cstim
Copy link
Contributor

cstim commented Jan 8, 2021

Describe the Bug
When using vrpc to wrap a C++ class into a nodejs addon, this will fail to compile (and work) if the C++ class is "non-copyable", i.e. its copy constructor is deleted. I encountered this while trying to wrap a C++ class that was using some thread and mutex members which are non-copyable.

Steps To Reproduce
In the file examples/Bar.hpp, add a deleted copy constructor in the class declaration, i.e. a line saying
Bar(const Bar&) = delete;

Then build the project by calling npm run build. Resulting compile error is this:

In file included from ../vrpc/addon.cpp:42:
../vrpc/vrpc.hpp: In instantiation of 'vrpc::Value::holder::holder(const T&) [with T = vrpc_example::Bar]':
../vrpc/vrpc.hpp:501:15: required from 'vrpc::Value::Value(const T&) [with T = vrpc_example::Bar]'
../vrpc/vrpc.hpp:445:21: required from 'std::string vrpc::to_string(const T&) [with T = vrpc_example::Bar; std::string = std::__cxx11::basic_string]'
../vrpc/vrpc.hpp:691:31: required from 'std::string vrpc::Value::holder<std::shared_ptr<_Tp> >::format() const [with T = vrpc_example::Bar; std::string = std::__cxx11::basic_string]'
../vrpc/vrpc.hpp:690:27: required from here
../vrpc/vrpc.hpp:643:14: error: use of deleted function 'vrpc_example::Bar::Bar(const vrpc_example::Bar&)'
643 | : held(new T(value)) { }
| ^~~~~~~~~~~~
In file included from ../examples/binding.cpp:2,
from ../vrpc/addon.cpp:45:
../examples/Bar.hpp:25:5: note: declared here
25 | Bar(const Bar&) = delete;
| ^~~
make: *** [vrpc_example.target.mk:109: Release/obj.target/vrpc_example/vrpc/addon.o] Error 1
make: Leaving directory '/home/cs/org/vrpc/build'

Expected Behavior
It is expected to be able to wrap a non-copyale C++ class, too. The compiler instantiation stack seems to indicate that the copy constructor was instantiated "only" for the to_string function and this seems unnecessary to me. The stack seems to indicate that the value object was wrapped in a shared_ptr anyway (line vrpc.hpp:691), but then from to_string the copy constructor of Value is called directly (line vrpc.hpp:445), which on the other hand seems rather weird to me. Maybe you need to do some tricks in to_string so that no copy constructor of Value is called?

Version
Current git master, i.e. version 2.2.3.

@cstim cstim added the bug Something isn't working label Jan 8, 2021
@cstim cstim changed the title Trying to wrapp a C++ class that is non-copyable will fail to compile Trying to wrap a C++ class that is non-copyable will fail to compile Jan 8, 2021
@cstim
Copy link
Contributor Author

cstim commented Jan 8, 2021

Update: The weird thing here is that the to_string instantiation with usage of operator<< triggers a Value<T> instantiation in turn, which will then fail.

I found out that this can be avoided if I define a operator<< for the actual wrapped class, i.e. in examples/Bar.hpp I additionally add the line:
std::ostream& operator<<(std::ostream& os, const Bar& value);
and then it actually compiles fine. So the issue can be fixed on the user side already, and maybe this just needs to be documented well enough. (Still weird as for why the Value<T> instantiation is triggered...)

@cstim cstim changed the title Trying to wrap a C++ class that is non-copyable will fail to compile Trying to wrap a C++ class that is non-copyable will fail to compile, unless a operator<< is defined for the class, too. Jan 8, 2021
@bheisen bheisen self-assigned this Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants