STL integrations isInstance bug

[stefanfrings]

From my understanding the STL container stack specializations provide the isInstance function to indicate if the element on the stack can be converted to the corresponding C++ container. This is the case if:

1. the object is a table
2. the size of the table is equal to the size of the container (only for std::array)
3. the types of all table elements are equal to the element type of the container

On the other hand the current implementation for all STL container stack specializations is as following:
static bool isInstance(lua_State* L, int index) { return lua_istable(L, index); }

The first criteria is checked by the implementation. The size check for std::array is trivial to implement. The last criteria can be checked as well but would require more performance. Is this behavior intended?

I suggest to either drop these isInstance functions or to implement them properly. Of course this does not affect the other isInstance functions.

Furthermore there are no tests for the functions available.

If I have some sparetime I can fix this issue but before I want to clarify/discuss the desired behavior.

[dmitry-t]

The main isInstance() idea is checking whether a Lua userdata contains an object of a C++ class.
Every stack specialization implements this method for the sake of consistency.

Checking the type of every table item means linear complexity which is slow and inconsistent with usual constant complexity.

The std::vector<T> is a class, it should be possible to implement the vector isInstance in the same manner as for other user classes. The only issue here is that a user class should be registered in a lua_State.

I start thinking that it would be a better idea to have a function that would register an std::vector instance class within a state, like this:

luabridge::registerStdVector<int>("vector");

v = std.vector({1, 2})
v:push_back(3)
print(v:at(0))

[stefanfrings]

Your idea solves the isInstance issue but I see other problems with this solution:

• Encapsulation: C++ implementation details leak into Lua source code (e.g. using a map or unordered_map).
• Performance considerations: I assume working with tables in Lua is more efficient than calling C++ functions with LuaBridge.
• Compatibility: Other Lua modules work most probably with Lua tables. Therefore the user has to perform this conversion anyway.
• Effort vs. advantage: Adding wrapper functions for all supported STL containers is quite a lot of effort and doesn't give a real advantage beside that the isInstance issue is fixed.

For simplicity and correctnes I tend to drop the STL container isInstance functions because they are not doing what they are supposed to do in contrast to the other isInstance functions.

Maybe another idea is to add a function isConvertible which checks every element and requires therefore more performance but I guess it's not required/desired at all.

[dmitry-t]

The change doesn't seem to be complicated (unless there are caveats).
But first we need Lua 5.3 and 5.4 unit tests.
And it'll be good to use github actions for tests.

[stefanfrings]

I still tend to drop all isInstance functions which can't guarantee that the corresponding get function can be called successfully.

• For most of the STL containers it's not feasible to implement isInstance without wasting performance for checking every element. Even see my previous comment about the alternative solution.
• For the number issue Crash in luabridge::Stack<int>::get #289 (comment) we can provide an isInstance function if Lua 5.2+ is available. Nevertheless earlier versions of Lua should be still supported, at least 5.1 is still quite popular e.g. LuaJIT.

If this way is chosen the isInstance functions which are going to be dropped should be marked as deprecated and removed in the next major release.

I absolutely agree that we require tests for Lua 5.3, 5.4 and even LuaJIT. At the moment LuaBridge doesn't work with LuaJIT but fixing this shouldn't be too much effort.

Furthermore I even welcome GitHub actions to run the tests automatically.