Skip to content

Migrating project from LuaBridge2 #83

Closed

Description

@rpatters1

I took a stab at migrating my project from LuaBridge2 to LuaBridge3. I ran into a couple of roadblocks:

  • After reading the fine manual, I do not see a non-intrusive option for a shared lifetime pointer of a class. The closest thing on offer appears to be std::shared_ptr, but it is not, in fact, non-intrusive. Per the manual, each such class has to derive std::enable_shared_from_this, unless I am missing something. Is there any reason I can't use RefCountedPtr from LuaBridge2? It seems like it should work without any changes.
  • After a brief mention in the executive summary at the top, CFunctions have no further mention in the manual. How do I register them in a namespace? The Namespace::addCFunction appears to be missing. (Or at least that's the error message I'm getting.)
  • When pushing return values on the Lua stack in a CFunction, I get an "ignoring return value of nodiscard function" warning. Ex.:
luabridge::Stack<bool>::push(L, returnVal);

What should I be doing with the return value? Or is there a more proper way to push values?

Activity

  1. Mellnik commented on Mar 31, 2023

    @Mellnik
    1. Juse use addFunction it detects it automatically.
    2. Yes, you have to handle the return value and not discard it. That's whats the [[nodiscard]] means.
  2. rpatters1 commented on Mar 31, 2023

    @rpatters1
    ContributorAuthor

    Thanks for replying. I know what nodiscard means. I'm just not sure what I'm supposed to do with the return value. Can you give an example of the correct code?

  3. rpatters1 commented on Mar 31, 2023

    @rpatters1
    ContributorAuthor

    FWIW: For the moment, I silenced the warning thusly:

    luabridge::Stack<CMPER>::push(l, returnVal.first) LB3(.operator bool());

    LB3 is a macro that strips out the code if it is compiling with OG LuaBridge.

    The [[nodiscard]] seems like an overly fussy requirement. The only situation it can fail is with the stack checking enabled. At the very least, it should be conditional on whether LUABRIDGE_SAFE_STACK_CHECKS is enabled.

  4. kunitoki commented on Apr 1, 2023

    @kunitoki
    Owner

    pushing on the stack can fail for multiple reasons and it's always safer to handle the case, you'll save yourself from additional grey hairs.

  5. kunitoki commented on Apr 1, 2023

    @kunitoki
    Owner

    Regarding non intrusive shared refcounting, luabridge RefCountedPtr is suboptimal design with architectural flaws (for example having an unprotected global static map of pointers is way unsafe and inefficient). If you still want to go that route, it's easy to roll your own, but it is not a solution the library should suggest as a preferred way to handle the case.

  6. rpatters1 commented on Apr 1, 2023

    @rpatters1
    ContributorAuthor

    stack pushing

    I would be interested in seeing the coding pattern for stack pushing that for you is optimal. If it involves an ugly sequence of if statements, I will push back with the observation that this is why we have luaL_error. :-)

    non-intrusive lifetime sharing

    Surely you agree that the library is of much more limited value if there is no way to have shared lifetime on a class without modifying it. (It would present a formidable barrier to using the library for my project, for example.) That said, I am not particularly in love with RefCountedPtr as implemented. Indeed, a couple of years ago I uncovered some flaws in it myself, which have subsequently been addressed.

    I am just brainstorming, but I'm wondering if it would be possible to re-implement RefCountedPtr as a wrapper around std::shared_ptr. Would that not get around the issues with type erasure? Just based on how RefCountedPtr works now, it seems like it might.

    using LuaBridge in a Clib

    One of the big problems with the current implementation of RefCountedPtr is that a Clib compiled with LuaBridge can't use it safely. That's because the Clib has its own version of the static global map. I don't know if any shared pointer scheme works in this situation, though.

    But this problem extends more fundamentally into LuaBridge. A Clib cannot safely register classes, either, at least not on LB2. They work just fine, but then the program crashes when the Lua state closes. I'm guessing there must be a hidden global somewhere, but I haven't ever bothered to go looking for it.

  7. kunitoki commented on Apr 1, 2023

    @kunitoki
    Owner

    Safety is the principle i'm adhering the library on, by relying on lua_error we are forcing the library to assume we have exceptions on (the lua panic handler cannot return or it aborts the application). I don't work like that and prefer to have the library to allow you to chose. You don't care ? write a lightweight wrapper and be fine with it. You want exception thrown or lua_error called from C++ ? write a ligthweight wrapper to throw your exceptions / lua errors in c++ land. But please don't force me to work this way (i can't, our game engine can't run with exceptions on for several reasons i won't explain in detail).

    // You dont' care ? then simply don't care
    template <class T>
    void push(lua_State* L, T&& value)
    {
        (void) luabridge::push(L, value);
    }
    
    // You care but you need exceptions ? throw
    template <class T>
    void push(lua_State* L, T&& value)
    {
        if (auto result = luabridge::push(L, value); not result)
            throw std::runtime_error(result.message());
    }
    
    // I can't use neither of the two. I care about safety but couldn't run with exceptions.

    Moreover Stack::push is a C++ api not a lua one so it needs to behave safely from a C++ context. I don't normally push a lot of object by hand but let luabridge do the push for me when i call binded methods / functions, so i don't care if i need to handle the errors explicitly. Error handling is nice and safe and it's way better than receiving some exception somewhere else in the code, that might or might not handle correctly the error in the first place (too generic, and you don't know how to handle it, too specific and you'll have try catch blocks as granular as checking the result value of push).

  8. rpatters1 commented on Apr 1, 2023

    @rpatters1
    ContributorAuthor

    I'm using Stack::push when I need my C function to return multiple values. If there is a bound way to do this, I would be happy to switch.

    How about adding a method to luabridge::Result that throws if there is an error. So the pattern could be:

    luabridge::push(L, value).throw_on_error();

    Or something like that.

  9. kunitoki commented on Apr 1, 2023

    @kunitoki
    Owner

    RefCountedPtr is broken. It's not scalable with thousands of objects (we have nearly hundred thousands), and doing lookups all the time is hitting badly a resonably big sized game, moreover those accesses are not thread safe which is a problem for us (and making it thread safe just increase contention which is slowing down the whole game). And yes, luabridge3 is safe to use across dll boundaries, and RefCountedPtr was preventing that safety from a library standpoint as well.

  10. kunitoki commented on Apr 1, 2023

    @kunitoki
    Owner

    How about adding a method to luabridge::Result that throws if there is an error. So the pattern could be:
    luabridge::push(L, value).throw_on_error();

    Sound like a great idea, will look into a proposal

  11. rpatters1 commented on Apr 1, 2023

    @rpatters1
    ContributorAuthor

    Are comments about RefCountedPtr in relation to the draft PR #85 I just posted? Or about the old one?

  12. kunitoki commented on Apr 1, 2023

    @kunitoki
    Owner

    Old one is broken, new one is incomplete as you cannot recover the reference count back safely into C++ land from lua.

  13. Repository owner locked and limited conversation to collaborators on Apr 1, 2023
  14. converted this issue into a discussion #87 on Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions