Open t-bltg opened 1 year ago
grasph
commented
1 year ago I don't remember where the issue comes from. The easiest way to start with to address it, is to comment this part of code and run wrapit on a function that takes a pass-by-r-value argument.
t-bltg
commented
1 year ago Thanks, well the generated code errors in libcxxwrap-julia, here is my toy code for the problem:
circle.hh
#include <iostream>
#include <utility>
using namespace std;
class Circle {
public:
Circle(int r)
{
_radius = r;
cout << "Standard constructor. Radius: " << _radius << endl;
}
Circle(const Circle& c)
{
_radius = c.radius();
cout << "Copy constructor with lvalue reference. Radius: " << _radius << endl;
}
Circle(Circle&& c)
{
_radius = c.radius();
cout << "Move constructor with rvalue reference. Radius: " << _radius << endl;
}
int radius() const {
return _radius;
}
private:
int _radius;
};
void test()
{
cout << "== from c++ ==" << endl;
Circle c1(2);
Circle c2(c1);
Circle c3(std::move(c2));
}conf.toml
module_name = "Circ"
input = ["circle.hh"]wrapit generated jlCirc.cxx
error log
grasph
commented
1 year ago It's a CxxWrap limitation. When do you want the move constructor to be called? It is not clear to me how you'd like to map the copy and move constructors to Julia.
t-bltg
commented
1 year ago Yes, it's unclear to me conceptually too how a move constructor would be called from the julia side, so it might be a dead end.
However, I'm wrapping a library that I'm in no position of modifying its sources. There, a class is declared where only a move constructor is defined so I'm kinda stuck here in my wrapping attempts.
grasph
commented
1 year ago I see. I'm adding @barche to get its opinion.
It's not clear to me know the move-only class should be handled, and more general pass-by-rvalue function parameters. The std::uniq_ptr is one notorious example of move-only class. As @t-bltg highlighted, Wrapit currently skip move-constructor and any function with a T&& parameter.
@t-bltg, if you give more insights on the class you want to wrap, this might help to understand use cases and how mapping to Julia should be done.
Philippe.
t-bltg
commented
1 year ago Thanks Philippe,
I cannot disclose the sources, but here is a reduced example of what is intended to be wrapped (you were dead-on right about std::unique_ptr usage):
#include <memory>
struct Options;
enum class State
{
IDLE = 0,
RUNNING = 1,
};
class CurrentState
{
public:
explicit CurrentState(std::unique_ptr<Options> && Options);
~CurrentState();
CurrentState(CurrentState const &) = delete;
CurrentState(CurrentState &&) = delete;
CurrentState & operator=(CurrentState const &) = delete;
CurrentState & operator=(CurrentState &&) = delete;
State getState() const
{ return m_state; }
private:
State m_state;
std::unique_ptr<Options> m_Options;
};Let's wait from @barche's inputs to see how to handle this case.
If you need an immediate solution, here is a workaround, you can use in the meantime.
Define a function in a header file you add to the WrapIt input list,
CurrentState createCurrentState(const Option& o){
return CurrentState(std::make_unique<Option>(o));
}Then, either require calling this function instead of the usual constructor or, for a more transparent use, define a constructor inside the julia module:
CurrentState(o::Option) = createCurrentState(o)To customize the julia module, you can use the pattern of ex002-ROOT: export list is generated in a separate file and the generated master file (limited to few lines) is discarded and replaced by a custom file. This will also allow you customizing the shared library path and solve at the same time the Issue #18.
Philippe.
t-bltg
commented
1 year ago That's a nice workaround !
I had to heap allocate and return a pointer, since the copy constructor was marked deleted in my example (hence failing to compile on return), but this is a detail unrelated to the initial issue.
Regarding https://github.com/grasph/wrapit/issues/18, I ended up writing my own module, however from a user point of view (a beginner using wrapit for the first time), I believe it might be better to use @__DIR__ since the generated module and the shared library reside in the same location.
barche
commented
1 year ago I think @grasph 's workaround is the only way for now. There is currently no way to directly wrap the move constructor, but it seems like it would be a nice feature to have. I think it should be possible to also provide a move function in CxxWrap to go along with it, to make sure the move constructor is called.
In https://github.com/grasph/wrapit/blob/3cb7ace5db9288007d75e5e443268f3e3df5cb9b/src/FunctionWrapper.cpp#L570-L579, it is stated that move constructors (taking a rvalue) are unsupported for the moment.
The generated code does not compile, right, but where should this be fixed ? Is it a limitation in
wrapit,CxxWraporlibcxxwrap-julia?I'd like to try to fix this, but I will need some pointers to start, so any comment would be very helpful.