Inheritance and shared_ptr as container. about luabridge3 HOT 10 CLOSED

Yes indeed, there might be something fishy in that static assert, in the case of enable_shared_from_this, the check fails. Will look into it, thanks for reporting.

from luabridge3.

This PR should fix it #52

from luabridge3.

Yep, works for me, thanks for quick fix!

Small note though, I didn't run tests, but just from quick look at PR's code, I see that you test B like this

EXPECT_TRUE(runLua("result = test.B(2)"));
auto a4 = result<std::shared_ptr<A>>();

That doesn't seem 100% correct to me, because looks like you get shared_ptr<A> and not shared_ptr<B>. For it to be tested correctly, I assume you would need some unique member in B that A doesn't have and then get shared_ptr<B> and access that specific member of B. But that would also require from you to do what I did in my example: overriding (or probably more correct term is hiding / shadowing, since it's not virtual, not sure) shared_from_this() method. So it would be something like

class B : public A
{
public:
    B() = default;

    B(int newX)
        : A(newX)
    {
    }
    
    int uniqueMethodOfB()
    {
        return 42;
    }
    
    std::shared_ptr<B> B::shared_from_this()
    {
        return std::static_pointer_cast<B>(A::shared_from_this());
    }
};

And then test calling B::uniqueMethodOfB() or do however you like it.

But if I think about it, that shouldn't make any difference for the actual fix. Looks like burden of overriding shared_from_this() should be on the shoulders of class developer anyway. Unless you want to specifically statically check in TypeTraits.h that this specific class being bound has its own shared_from_this() method that returns exactly this class pointer.

from luabridge3.

Yes thanks, makes sense. I've added something that should mitigate the need of having to override shared_from_this.

I noticed that if type is polymorphic, it should be better to use dynamic_cast and allow the rtti determine the if the type could be casted. While testing that i realized that when using getGlobal (or Stack directly) which indirectly uses Userdata::get, that method will check the type internally in the luabridge class tables and if it can't convert it, it calls the lua panic handler. Problem is, we called getGlobal from C++ not lua, so we end up with a program termination when not building with exceptions.

from luabridge3.

I see. Well, I don't know what to recommend here, probably calling panic handler is correct thing to do, and it's up to LuaBridge user to watch with which types he works to avoid it. If I understand correctly, normally it should work fine when there is inheritance between classes, either polymorphic or not.

Speaking of inheritance, I noticed one more thing yesterday, looks like static_assert doesn't work anymore with classes without it, can you please check that? Previously in my code I used only two classes with std::enabled_shared_from_this, where B inherited from A, and yesterday I added one more class without any inheritance, well, except inheritance from std::enabled_shared_from_this, of course, and looks like is_base_of_template_v didn't work well on it.

from luabridge3.

Do you have an example that fails ?

from luabridge3.

Hm, weird, can't reproduce it anymore. Yesterday, when I had some error about it, I commented that static_assert line again, and it worked. And now I uncommented it, I still have these same classes without inheritance and code compiles without errors. So there's a chance that was false alarm or I messed up something in my code.

I also verified that bindings work correctly without shared_from_this() override, so I think this issue may be closed now.
Thanks again!

from luabridge3.

Sorry, I will need to reopen this issue, although thing I encountered doesn't have to do anything with inheritance, but it's still related to using shared_ptr as container, so I guess it's better to post it here rather than creating new issue.
And that's not critical, because there's easy workaround, but if it can be fixed, it would be nice.

So, something's wrong with binding properties of a class which are shared_ptrs, and this problem appears only when you bind property as plain field, and not when as property with getter method.

Minimum code example should be this:

// Test.h
#include <memory>

class Test
{
public:
  class Nested : public std::enable_shared_from_this<Nested>
  {
  public:
    inline int getValue() const { return 42; }
  };

  Test();

  inline std::shared_ptr<Nested> getSpNested() const { return spNested; }

  static void bindToLua();

private:
  Nested nested;
  std::shared_ptr<Nested> spNested;
};

// Test.cpp
#include "luaState.h"
#include "luabridge/LuaBridge.h"
#include "Test.h"

using namespace luabridge;

Test::Test()
{
  spNested = std::make_shared<Nested>();
}

void Test::bindToLua()
{
  const auto L = getLuaState();

  if (!getGlobal(L, "Test").isTable())
  {
    getGlobalNamespace(L)
      .beginClass<Test>("Test")
        .addConstructor<void()>()
        .addProperty("nestedAsField", &Test::nested)
        .addProperty("spNestedAsField", &Test::spNested)
        .addProperty("spNestedAsFuncProp", &Test::getSpNested)
      .endClass()
      .beginClass<Test::Nested>("TestNested")
        .addFunction("getValue", &Test::Nested::getValue)
      .endClass();
  }
}

By the way, using nested class here is unrelated, it can be any class, here it's nested just for simplicity.

And then what's interesting, if you try to access test.nestedAsField in Lua, it works no problem, because it's simple class instance, not shared_ptr, but when you access test.spNestedAsField, which is shared_ptr, it fails without any error description. At the same time binding this property as getter method, i.e. test.spNestedAsFuncProp, works no problem.
So in Lua it looks like:

local test = Test()
print(test.nestedAsField:getValue()) -- Good.
print(test.spNestedAsFuncProp:getValue()) -- Good.
print(test.spNestedAsField:getValue()) -- Fails here.

Just in case, I updated my previous example code with that new Test class - https://www.dropbox.com/s/qplxkfmmtpbhyla/SharedFromThisTest.zip?dl=0.

from luabridge3.

Fixed in #57

from luabridge3.

Confirmed, thanks!

from luabridge3.