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

Extensible classes support #116

Merged
merged 26 commits into from
Apr 21, 2023
Merged

Extensible classes support #116

merged 26 commits into from
Apr 21, 2023

Conversation

kunitoki
Copy link
Owner

This PR aims at allowing C++ binded classes to be extensible from lua:

    luabridge::getGlobalNamespace(L)
        .beginClass<X>("X", luabridge::extensibleClass)
            .addConstructor<void(*)()>()
        .endClass();

    int result = runLua(R"(
        function X:test()
            return 1
        end

        local x = X()
        result = x:test()
    )");

    assert(result == 1);

@kunitoki kunitoki linked an issue Apr 13, 2023 that may be closed by this pull request
@rpatters1
Copy link
Contributor

If a base class sets this option, will all subclasses be extensible as well? (Just trying to understand what is being added.)

@kunitoki
Copy link
Owner Author

This is something i'm working on. Extensibility is a class property, so should be enabled in each class individually.

@rpatters1
Copy link
Contributor

Extensibility is a class property, so should be enabled in each class individually.

Yes, I agree. I suppose the better, more precise question is, will subclasses be able to access methods added to one of its base classes (up the hierarchy)?

@kunitoki
Copy link
Owner Author

@rpatters1
Copy link
Contributor

rpatters1 commented Apr 13, 2023

I'm assuming these functions all receive a self parameter. It might be good to include some tests around that.

@kunitoki
Copy link
Owner Author

I'm assuming these functions all receive a self parameter. It might be good to include some tests around that.

Definately, this is just the beginning, a lot more tests are needed down there.

@rpatters1
Copy link
Contributor

Another point of clarification. It looks like the answer is "no" but just to confirm:

Does this allow you to replace existing functions that were registered on the class? One of the reasons we added the mixin library was to improve the DX around clunky APIs in the original C++ classes. An example is that the C++ framework has its own homegrown string class, and in order to pass text to any of those classes, you have to stop and create one of the proprietary string types, stuff your text into it, then pass it. The mixin library replaces those methods with methods that take anything it can recognize as a string.

@kunitoki
Copy link
Owner Author

kunitoki commented Apr 13, 2023

I'm thinking this by default should not be allowed (added a check for this here 810813e#diff-a6c1434d943944028ce926611075458078b52d6d2de84188f557c344c3c0fe74R297-R313 which prevents any modification).

But i see the use cases in controlled environments (we might have a use for that as well for the reason you mention). One complication for that feature could be allowing still to access the super() method being wrapped, in order to properly "proxy" it.

@ThistleSifter
Copy link

Will this also eventually include the ability to create custom subclasses from extensible classes?

@kunitoki
Copy link
Owner Author

kunitoki commented Apr 14, 2023

Not sure i would mix luabridge inheritance (that works on top of c++ userdata) with mixed one (a lua table which parent is a c++ userdata), mostly because it will overcomplicate key resolution. But contributions are welcome. I suggest we start a discussion on how we would like this to work.

Use composition over inheritance, the world is bad already.

@kunitoki
Copy link
Owner Author

kunitoki commented Apr 15, 2023

Does this allow you to replace existing functions that were registered on the class?

This is now in

TEST_F(ClassTests, ExtensibleClassExtendExistingMethodAllowingOverride)
{
luabridge::getGlobalNamespace(L)
.beginClass<ExtensibleBase>("ExtensibleBase", luabridge::extensibleClass | luabridge::allowOverridingMethods)
.addConstructor<void(*)()>()
.addFunction("baseClass", &ExtensibleBase::baseClass)
.addFunction("baseClassConst", &ExtensibleBase::baseClassConst)
.endClass()
;
runLua(R"(
function ExtensibleBase:baseClass() return 42 + self:super_baseClass() end
local base = ExtensibleBase(); result = base:baseClass()
)");
EXPECT_EQ(43, result<int>());
runLua(R"(
function ExtensibleBase:baseClassConst() return 42 + self:super_baseClassConst() end
local base = ExtensibleBase(); result = base:baseClassConst()
)");
EXPECT_EQ(44, result<int>());
}

I would maybe later enable separate index and newindex metamethods for extensible classes, so to not incur into a performance penalty for classes that are not extensible.

Currently, there are some problems with the super_ methods and inherited classes

@kunitoki kunitoki linked an issue Apr 16, 2023 that may be closed by this pull request
@kunitoki kunitoki merged commit f32f165 into master Apr 21, 2023
@kunitoki kunitoki deleted the dev/extensible_classes branch April 21, 2023 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend bound classes with custom methods in Lua Question about luabridge::setHideMetatables
3 participants