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

Reusing nim method for wrapping C++ methods #1

Open
haxscramper opened this issue Jun 6, 2021 · 11 comments
Open

Reusing nim method for wrapping C++ methods #1

haxscramper opened this issue Jun 6, 2021 · 11 comments
Labels
C++ Issues related specifically to C++ processing open for design discussion question Further information is requested

Comments

@haxscramper
Copy link
Owner

haxscramper commented Jun 6, 2021

When interfacing with C++ classes and methods, it is necessary to re-wrap methods as procedures for each derived class. E.g. if I have Base and Derived I would have to repeat wrapping of the Base methods for Derived type. I tried using method, but it gives me 'invalid pragma: importcpp: "#.toOverride(@)"' error on the following code, so it looks like I can't benefit from nim OOP features in this case.

  {.emit: """/*TYPESECTION*/
struct Base {
  int baseField;
  int baseMethod() { return 12; }
  virtual int toOverride() { return 42; }

};
struct Derived : public Base {
  int derivedField;
  int toOverride() override { return 45; }
};
""".}

type
  CxxBase {.importcpp: "Base", bycopy, inheritable.} = object
    baseField: cint

  CxxDerived {.importcpp: "Derived", bycopy.} = object of CxxBase
    derivedField: cint

# All methods of the base class must be repeated for each derived. For
# cases like `QWidget` that has 214 base methods and 37 derived it classes results
# in **7918!** repetitions.
proc baseMethod(base: CxxBase): cint {.importcpp: "#.baseMethod(@)".}
proc baseMethod(base: CxxDerived): cint {.importcpp: "#.baseMethod(@)".}

proc toOverride(base: CxxBase): cint {.importcpp: "#.toOverride(@)".}
proc toOverride(base: CxxDerived): cint {.importcpp: "#.toOverride(@)".}

proc toOverrideTypeclass(base: CxxBase | CxxDerived): cint {.importcpp: "#.toOverride(@)".}


# Ideally I would like to import C++ methods as **methods**, but I get
# 'Error: invalid pragma: importcpp: "#.toOverride(@)"'

when false:
  method toOverrideMethod(base: CxxBase): cint {.importcpp: "#.toOverride(@)".}

block:
  let derived = CxxDerived(baseField: 123123)
  echo derived.baseField

  echo CxxBase().toOverride()
  echo CxxDerived().toOverride()

  echo CxxBase().toOverrideTypeclass()
  echo CxxDerived().toOverrideTypeclass()

type
  NimBase {.inheritable.} = object
    baseField: cint

  NimDerived = object of NimBase
    derivedField: cint

method baseMethod(base: NimBase): int {.base.} = 12
method toOverride(base: NimBase): int {.base.} = 42
method toOverride(base: NimDerived): int = 45

block:
  let derived = NimDerived(baseField: 123123)
  echo derived.baseField

  echo NimBase().toOverride()
  echo NimDerived().toOverride()

I specifically repeated the implementation in nim as well to illustrate how I would like wrapper to behave. It is not difficult to generate additional wrappers for all derived classes, but as indicated in comment it could lead to multi-thousand repetitions (again QWidget has 37 derived classes, and it would repeat at least 214 methods for each of them. And I used QWidget as and example. With QObject it could be even bigger).

I thought about possible solutions, but they don't seem particularly plausible to me:

  1. Using typeclasses - proc toOverride(base: CxxBase | CxxDerived): cint {.importcpp: "#.toOverride(@)".}. It might look fine for such a small example, but in the end it would mean putting all derived class wrappers in a single file, and declare all procedures afterwards, creating one giant file from a library instead of keeping things separated.
  2. Implementing wrappers as a macro that checks if the argument has been derived from required base class - this might work, but it seems like a horrible hack where I have to mix {.emit.} with macro code and traverse full inheritance tree on each call encountered in code.

So the question is - can method somehow be used to solve this issue, or I need to generate repeated wrappers for each derived class (right now I'm doing this, which results in tons of repetitions)?


Originally asked in forum thread, but also copied here because it is particularly important, but unlikely to be answered soon.

@haxscramper haxscramper added the question Further information is requested label Jun 6, 2021
@timotheecour
Copy link

the following "almost works":

when true:
  {.emit: """/*TYPESECTION*/
  struct Base {
    int baseField;
    int baseMethod() { return 12; }
    virtual int toOverride() { return 42; }

  };
  struct Derived : public Base {
    int derivedField;
    int toOverride() override { return 45; }
  };
  """.}

  type
    CxxBase {.importcpp: "Base", bycopy, inheritable.} = object
      baseField: cint

    CxxDerived {.importcpp: "Derived", bycopy.} = object of CxxBase
      derivedField: cint

  proc baseMethodImpl(base: CxxBase): cint {.importcpp: "#.baseMethod(@)".}
  method baseMethod(base: CxxBase): cint {.base.} = baseMethodImpl(base)
  proc toOverrideImpl(base: CxxBase): cint {.importcpp: "#.toOverride(@)".}
  method toOverride(base: CxxBase): cint {.base.} = toOverrideImpl(base)
  when defined case2b:
    proc toOverrideImpl2(base: CxxDerived): cint {.importcpp: "#.toOverride(@)".}
    method toOverride(base: CxxDerived): cint = toOverrideImpl2(base)

  block:
    let derived = CxxDerived(baseField: 123123)
    echo derived.baseField

    echo CxxBase().baseMethod()
    echo CxxDerived().baseMethod()

    echo CxxBase().toOverride()
    when defined case2b:
      echo CxxDerived().toOverride()

nim r -b:cpp main works
nim r -b:cpp -d:case2b main gives cgen error:

/Users/timothee/git_clone/nim/timn/build/nimcache/@mt12445.nim.cpp:222:14: error: no member named 'm_type' in 'Base'
                if (!(base.m_type == (&NTIcxxderived__5KCvJiTlz3Lz1sFqYEsBSQ_))) goto LA3_;
                      ~~~~ ^
/Users/timothee/git_clone/nim/timn/build/nimcache/@mt12445.nim.cpp:223:20: error: no member named 'm_type' in 'Base'
{               if (!isObj(base.m_type, (&NTIcxxderived__5KCvJiTlz3Lz1sFqYEsBSQ_))){ raiseObjectConversionError(); }
                           ~~~~ ^
/Users/timothee/git_clone/nim/timn/build/nimcache/@mt12445.nim.cpp:230:29: error: no member named 'm_type' in 'Base'
                if (!(isObjWithCache(base.m_type, (&NTIcxxbase__AnOdMbGZliGJzFUoc6fCJA_), Nim_OfCheck_CACHE3))) goto LA6_;
                                     ~~~~ ^

@haxscramper
Copy link
Owner Author

haxscramper commented Jun 21, 2021

This issue can be partially solved if only ref/ptr types are used. It still breaks on the value objects as things just break on C++ semantics (when passing bycopy objects to a procedure)

{.emit: """/*TYPESECTION*/
struct Base {
  int baseField;
  int baseMethod() { return 12; }
  virtual int toOverride() { return 42; }

};
struct Derived : public Base {
  int derivedField;
  int toOverride() override { return derivedField; }
};

struct Other {};
""".}

type
  CxxBase {.importcpp: "Base", bycopy, inheritable.} = object
    baseField: cint

  CxxDerived {.importcpp: "Derived", bycopy.} = object of CxxBase
    derivedField: cint

  CxxOther {.importcpp: "Other", bycopy.} = object


proc aux() {.header: "<new>", importc: "//".}

proc toOverride(base: ref CxxBase): cint {.importcpp: "#->toOverride(@)".}
  # {.emit: "return `base`->toOverride();".}

proc newCxxBase(): ref CxxBase  =
  aux()
  new(result)
  {.emit: "new ((void*)result) Base();".}

proc newCxxDerived(): ref CxxDerived  =
  aux()
  new(result)
  {.emit: "new ((void*)result) Derived();".}

proc newCxxOther(): ref CxxOther =
  aux()
  new(result)
  {.emit: "new ((void*)result) Other();".}

echo newCxxBase().toOverride()
echo newCxxDerived().toOverride()
# echo newCxxOther().toOverride()

@haxscramper
Copy link
Owner Author

Actually I just realized I can drop bycopy and replace it with byref:

{.emit: """/*TYPESECTION*/
struct Base {
  int baseField;
  int baseMethod() { return 12; }
  virtual int toOverride() { return 42; }

};
struct Derived : public Base {
  int derivedField;
  int toOverride() override { return derivedField; }
};

struct Other {};
""".}

type
  CxxBase {.importcpp: "Base", byref, inheritable.} = object
    baseField: cint

  CxxDerived {.importcpp: "Derived", byref.} = object of CxxBase
    derivedField: cint

  CxxOther {.importcpp: "Other", byref.} = object

proc aux() {.header: "<new>", importc: "//".}

method toOverride(base: CxxBase): cint {.base.} =
  {.emit: "return `base`->toOverride();".}

echo CxxBase().toOverride()
echo CxxDerived().toOverride()
# echo newCxxOther().toOverride()

now works corectly

@timotheecour
Copy link

cool!
nim manual is in desperate need for a beefed up section (ideally a separate rst file, eg interop.rst) dedicated to interop, are you interested in a PR that would show this example?

needs a bit of cleanup, eg remove aux and add

  var a = CxxDerived()
  a.derivedField = 123
  assert a.toOverride() == 123

@haxscramper
Copy link
Owner Author

Feel free to add it to any manual section, but after releasing alpha version of hcparse I wanted to spend some time summarizing all my solution in a series of articles, something like "Nim for C++ programmer". Also, I'm not really sure I'd this is the correct approach, so I'd prefer to mass-test it on some library (like Qt, which was the reason for this question) and then say "yes, this is the correct way that would not bite you on some edge case"

@timotheecour
Copy link

timotheecour commented Jun 21, 2021

  • the correct way should (IMO) still involve supporting method+importcpp and avoid resorting to emit which has its own issues, eg not working in templates (i can link the issue if needed), eg:
template fn =
  method toOverride(base: CxxBase): cint {.base.} =
    {.emit: "return `base`->toOverride();".} # needs emit + array syntax for this to work currently

but in the meantime it's good to show what's possible to unblock ppl that need interop; it's more of a living document given that C++ interop is still in flux

  • can you test more cases eg when nim code defines method overrides (both with importcpp objects and with nim objects like CxxDerived2):
type
  CxxDerived2 = object of CxxDerived
    derivedField2: cint
method toOverride(base: CxxDerived2): cint = 33

?

@haxscramper
Copy link
Owner Author

haxscramper commented Jun 21, 2021

method toOverride(derived: CxxDerived2): cint =
  echo "Overide method impementation from nim side"
  return 1231.cint

echo newCxxDerived2().toOverride()

does not work as it fails with C++ codegen compilation error

/mnt/workspace/clean-clone/hack/testing/nim/c_interop/cpp_tests/cache/@minheritance.nim.cpp:330:31: error: ‘struct Base’ has no member named ‘m_type’
  330 |                 if (!((*base).m_type == (&NTI__5WdQBNAaXBHGF2AaoSDNJQ_))) goto LA3_;

@timotheecour
Copy link

timotheecour commented Jun 21, 2021

i see, so it's same error as above (#1 (comment))

maybe file an issue in nim repo? RFC or not RFC, this should be fixed (and bugs with a tracking issue are more likely to be addressed, at very least gives a centralized placeholder for discussion)

@haxscramper
Copy link
Owner Author

haxscramper commented Jun 21, 2021

Well, the issue seems obvious - object is not derived from a RootObj so it does not have an m_type field. Since I also want to derive from C++ issue it would be unsolvable without multiple inheritance.

So I will refrain from filing issues on this since it would be incoherent noise IMO, this need to be addressed in an RFC

@timotheecour
Copy link

timotheecour commented Jun 21, 2021

it would be unsolvable without multiple inheritance.

there should be a solution for this case that doesn't require solving MI (which is more complex and can be addressed seperately), ie allowing (possibly with a pragma) method toOverride(base: CxxDerived2): cint = 33 to work even though CxxDerived2 doesn't derive from a RootObj; ie deriving from an importcpp inheritable object should be enough

(and yes, an RFC would be good)

@haxscramper haxscramper added C++ Issues related specifically to C++ processing open for design discussion labels Oct 6, 2021
haxscramper added a commit that referenced this issue Nov 3, 2021
- CHANGED ::
  - Class wrappers now include wrappers for the superclass methods as well.
    Current implementation simply copy-pastes all wrapped types once more,
    but ideally this should be optimized, either using
    #1 with `{.byref.}` hack,
    or by a proper language support.
@haxscramper
Copy link
Owner Author

Alternatively to hacks (generating byref types or bruteforce solutions with copypasting every method) I can just require explicit type conversion using 'as' operator -

var slider = cnewQSlidwe()
slider.as(QWidget)[].someQWidetMethod()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ Issues related specifically to C++ processing open for design discussion question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants