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

Support for T&& function parameters and the special move constructor case #24

Open
t-bltg opened this issue Mar 10, 2023 · 9 comments
Open

Comments

@t-bltg
Copy link
Contributor

t-bltg commented Mar 10, 2023

In

if(rvalueref_arg){
//The code generated for a function with an argument passed as a r-value
//reference does not compile.
//Until, it is fixed, skipped these functions
std::cerr << "Warning: no wrapper will be produced for function '"
<< name_cxx << "(" << short_arg_list_cxx << ") " << cv
<< "' because it contains an argument passed by r-value "
<< "which is not supported yet.\n";
return false;
}
, 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, CxxWrap or libcxxwrap-julia ?

I'd like to try to fix this, but I will need some pointers to start, so any comment would be very helpful.

@grasph
Copy link
Owner

grasph commented Mar 10, 2023

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

t-bltg commented Mar 10, 2023

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

#include <type_traits>
#include "jlcxx/jlcxx.hpp"
#include "jlcxx/functions.hpp"
#include "jlcxx/stl.hpp"

#include "jlCirc.h"

#ifdef VERBOSE_IMPORT
#  define DEBUG_MSG(a) std::cerr << a << "\n"
#else
#  define DEBUG_MSG(a)
#endif
#define __HERE__  __FILE__ ":" QUOTE2(__LINE__)
#define QUOTE(arg) #arg
#define QUOTE2(arg) QUOTE(arg)

namespace jlcxx {
  template<> struct IsMirroredType<Circle> : std::false_type { };
  template<> struct DefaultConstructible<Circle> : std::false_type { };
}

JLCXX_MODULE define_julia_module(jlcxx::Module& types){

  DEBUG_MSG("Adding wrapper for type Circle (" __HERE__ ")");
  // defined in ./circle.hh:6:7
  auto t0 = types.add_type<Circle>("Circle");

  /**********************************************************************/
  /* Wrappers for the methods of class Circle
   */


  DEBUG_MSG("Adding wrapper for void Circle::Circle(int) (" __HERE__ ")");
  // defined in ./circle.hh:8:5
  t0.constructor<int>(/*finalize=*/true);


  DEBUG_MSG("Adding wrapper for void Circle::Circle(const Circle &) (" __HERE__ ")");
  // defined in ./circle.hh:14:5
  t0.constructor<const Circle &>(/*finalize=*/true);


  DEBUG_MSG("Adding wrapper for void Circle::Circle(Circle &&) (" __HERE__ ")");
  // defined in ./circle.hh:20:5
  t0.constructor<Circle &&>(/*finalize=*/true);

  DEBUG_MSG("Adding wrapper for int Circle::radius() (" __HERE__ ")");
  // signature to use in the veto list: int Circle::radius()
  // defined in ./circle.hh:26:9
  t0.method("radius", static_cast<int (Circle::*)()  const>(&Circle::radius));

  /* End of Circle class method wrappers
   **********************************************************************/


  /**********************************************************************
   * Wrappers for global functions and variables including
   * class static members
   */

  DEBUG_MSG("Adding wrapper for void test() (" __HERE__ ")");
  // signature to use in the veto list: void test()
  // defined in ./circle.hh:34:6
  types.method("test", static_cast<void (*)() >(&test));

  /* End of global function wrappers
   **********************************************************************/

  DEBUG_MSG("End of wrapper definitions");

}

error log

In file included from jlCirc.cxx:2:
In file included from [...]/include/jlcxx/jlcxx.hpp:15:
[...]/include/jlcxx/module.hpp:72:14: error: no matching function for call to object of type 'ReturnTypeAdapter<jlcxx::BoxedValue<Circle>, Circle &&>'
      return ReturnTypeAdapter<R, Args...>()(functor, args...);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[...]/include/jlcxx/module.hpp:209:69: note: in instantiation of member function 'jlcxx::detail::CallFunctor<jlcxx::BoxedValue<Circle>, Circle &&>::apply' requested here
    return reinterpret_cast<void*>(detail::CallFunctor<R, Args...>::apply);
                                                                    ^
[...]/include/jlcxx/module.hpp:196:3: note: in instantiation of member function 'jlcxx::FunctionWrapper<jlcxx::BoxedValue<Circle>, Circle &&>::pointer' requested here
  FunctionWrapper(Module* mod, const functor_t &function) : FunctionWrapperBase(mod, julia_return_type<R>()), m_function(function)
  ^
[...]/include/jlcxx/module.hpp:526:29: note: in instantiation of member function 'jlcxx::FunctionWrapper<jlcxx::BoxedValue<Circle>, Circle &&>::FunctionWrapper' requested here
    auto* new_wrapper = new FunctionWrapper<R, Args...>(this, f);
                            ^
[...]/include/jlcxx/module.hpp:700:12: note: in instantiation of function template specialization 'jlcxx::Module::method<jlcxx::BoxedValue<Circle>, Circle &&>' requested here
    return method(name, std::function<R(ArgsT...)>(std::forward<LambdaT>(lambda)));
           ^
[...]/include/jlcxx/module.hpp:555:12: note: in instantiation of function template specialization 'jlcxx::Module::add_lambda<jlcxx::BoxedValue<Circle>, (lambda at [...]/include/jlcxx/module.hpp:562:67), Circle &&>' requested here
    return add_lambda(name, std::forward<LambdaT>(lambda), &LambdaT::operator());
           ^
[...]/include/jlcxx/module.hpp:562:51: note: in instantiation of function template specialization 'jlcxx::Module::method<(lambda at [...]/include/jlcxx/module.hpp:562:67)>' requested here
    FunctionWrapperBase &new_wrapper = finalize ? method("dummy", [](ArgsT... args) { return create<T, true>(args...); }) : method("dummy", [](ArgsT... args) { return create<T, false>(args...); });
                                                  ^
[...]/include/jlcxx/module.hpp:986:16: note: in instantiation of function template specialization 'jlcxx::Module::constructor<Circle, Circle &&>' requested here
      m_module.constructor<T, ArgsT...>(m_dt, finalize);
               ^
jlCirc.cxx:45:6: note: in instantiation of function template specialization 'jlcxx::TypeWrapper<Circle>::constructor<Circle &&>' requested here
  t0.constructor<Circle &&>(/*finalize=*/true);
     ^
[...]/include/jlcxx/module.hpp:43:22: note: candidate function not viable: expects an rvalue for 2nd argument
  inline return_type operator()(const void* functor, static_julia_type<Args>... args)
                     ^
1 error generated.

@grasph
Copy link
Owner

grasph commented Mar 10, 2023

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

t-bltg commented Mar 11, 2023

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 grasph changed the title move constructor Support for T&& function parameters and the special move constructor case Mar 11, 2023
@grasph
Copy link
Owner

grasph commented Mar 11, 2023

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

t-bltg commented Mar 11, 2023

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;
};

@grasph
Copy link
Owner

grasph commented Mar 11, 2023

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

t-bltg commented Mar 11, 2023

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

barche commented Mar 12, 2023

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.

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

3 participants