JuliaInterop / libcxxwrap-julia

C++ library for backing CxxWrap.jl
Other
84 stars 43 forks source link

Regression with 0.12.3 in the support of static inheritance #156

Closed grasph closed 5 months ago

grasph commented 5 months ago

Hello Bart,

with libcxxwrap_julia_jll 0.12.3+, using SuperType for static inheritance (classes no virtual function) leads to a compilation error,

In file included from /home/pgras/.julia/artifacts/61238de4948b5e57c436ccdfea5e7ffcda1913b2/include/jlcxx/jlcxx.hpp:15,
                 from /home/pgras/dev/julia/wrapit/test/issue_cxxwrap-v0.15.1/inheritance.cxx:1:
/home/pgras/.julia/artifacts/61238de4948b5e57c436ccdfea5e7ffcda1913b2/include/jlcxx/module.hpp: In instantiation of ‘static void jlcxx::DownCast<SuperT, DerivedT>::apply(jlcxx::Module&) [with SuperT = A; DerivedT = B]’:
/home/pgras/.julia/artifacts/61238de4948b5e57c436ccdfea5e7ffcda1913b2/include/jlcxx/module.hpp:1054:36:   required from ‘void jlcxx::add_default_methods(Module&) [with T = B]’
/home/pgras/.julia/artifacts/61238de4948b5e57c436ccdfea5e7ffcda1913b2/include/jlcxx/module.hpp:1316:27:   required from ‘jlcxx::TypeWrapper<T> jlcxx::Module::add_type_internal(const std::string&, JLSuperT*) [with T = B; SuperParametersT = jlcxx::ParameterList<>; JLSuperT = _jl_datatype_t; std::string = std::__cxx11::basic_string<char>]’
/home/pgras/.julia/artifacts/61238de4948b5e57c436ccdfea5e7ffcda1913b2/include/jlcxx/module.hpp:1327:48:   required from ‘jlcxx::TypeWrapper<T> jlcxx::Module::add_type(const std::string&, JLSuperT*) [with T = B; SuperParametersT = jlcxx::ParameterList<>; JLSuperT = _jl_datatype_t; std::string = std::__cxx11::basic_string<char>]’
/home/pgras/dev/julia/wrapit/test/issue_cxxwrap-v0.15.1/inheritance.cxx:19:23:   required from here
/home/pgras/.julia/artifacts/61238de4948b5e57c436ccdfea5e7ffcda1913b2/include/jlcxx/module.hpp:1038:82: error: cannot ‘dynamic_cast’ ‘base’ (of type ‘struct A*’) to type ‘struct B*’ (source type is not polymorphic)
 1038 |     mod.method("cxxdowncast", [](SingletonType<DerivedT>, SuperT* base) { return dynamic_cast<DerivedT*>(base); });
....

Code to reproduce the issue:

#include "jlcxx/jlcxx.hpp"

struct A {
};

struct B: public A {
};

namespace jlcxx {
  template<> struct IsMirroredType<A> : std::false_type { };
  template<> struct IsMirroredType<B> : std::false_type { };
  template<> struct SuperType<B> { typedef A type; };
}

JLCXX_MODULE define_julia_module(jlcxx::Module& jlModule){
  jlModule.add_type<A>("A");
  jlModule.add_type<B>("B", jlcxx::julia_base_type<A>());
}

I started to get this problem with CxxWrap 0.15.1. But if I install now CxxWrap 0.15.0, libcxxwrap_jll 0.12.3+0 is installed by default. Is it expected, should it be 0.12.2, which I expect to be the version installed before 0.12.3 was released?

(build) pkg> add CxxWrap@0.15.0
   Resolving package versions...
    Updating `~/dev/julia/wrapit/test/issue_cxxwrap-v0.15.1/build/Project.toml`
⌃ [1f15a43c] + CxxWrap v0.15.0
    Updating `~/dev/julia/wrapit/test/issue_cxxwrap-v0.15.1/build/Manifest.toml`
⌃ [1f15a43c] + CxxWrap v0.15.0
  [692b3bcd] + JLLWrappers v1.5.0
  [1914dd2f] + MacroTools v0.5.13
  [21216c6a] + Preferences v1.4.3
  [3eaa8342] + libcxxwrap_julia_jll v0.12.3+0
  [56f22d72] + Artifacts
  [2a0f44e3] + Base64
  [ade2ca70] + Dates
  [8f399da3] + Libdl
  [d6f4376e] + Markdown
  [de0858da] + Printf
  [9a3f8284] + Random
  [ea8e919c] + SHA v0.7.0
  [9e88b42a] + Serialization
  [fa267f1f] + TOML v1.0.3
  [4ec0a83e] + Unicode
        Info Packages marked with ⌃ have new versions available and may be upgradable.

cc: @peremato who brought my attention to this issue.

Philippe.

grasph commented 5 months ago

See comment on this code here

barche commented 5 months ago

Thanks, I'll try to look into this tomorrow, most likely we can fix this by distinguishing between the dynamic and static cases using std::is_polymorphic and a new minor release of libcxxwrap_julia will solve it.

barche commented 5 months ago

This should be fixed in the just released libcxxwrap_julia 0.12.5

grasph commented 5 months ago

Thanks a lot. I confirm it fixes the issue.