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

LuaRuntime.table_from method #34

Merged
merged 9 commits into from
Nov 2, 2014
Merged

LuaRuntime.table_from method #34

merged 9 commits into from
Nov 2, 2014

Conversation

kmike
Copy link
Collaborator

@kmike kmike commented Nov 1, 2014

Implementation of #31.

py_to_lua(self, L, arg)
lua.lua_rawseti(L, -2, i+1)
for obj in args:
if isinstance(obj, dict):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyMapping_Check() doesn't work as expected in Python 3.x, see http://bugs.python.org/issue5945. I've used isinstance(obj, dict) - is there a better way to do it?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, good call. Then checking for dicts is ok. It's easy enough to convert a non-dict mapping to a dict.

We could also make it configurable by accepting keyword arguments "mapping_types" and "sequence_types" that would default to dict and (tuple, list) respectively, and then do an isinstance() check with them (unless it's None). First check for the sequence types and only failing that, check for the mapping types. That way you can pass None for any of them to disable the conversion completely, or object to enable a specific conversion for all types, or any of the ABC classes, or whatever users want to come up with.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or why not go for ABCs directly? The type check on the each item isn't performance critical - any non-empty sequence or mapping will require way more time to copy than that.

@kmike kmike changed the title LuaRuntime.table_from method [WIP] LuaRuntime.table_from method Nov 1, 2014
@kmike
Copy link
Collaborator Author

kmike commented Nov 1, 2014

Do you think table_from should support nested dicts?

@scoder
Copy link
Owner

scoder commented Nov 1, 2014

Do you think table_from should support nested dicts?

Good idea. Then we should remove the generic support for **kwargs and
instead reserve keyword arguments to table_from() to configure the
conversion. Passing "recursive=True" would then do a recursive conversion
of dicts of lists of dicts of ...

I also think that the "_LuaTable" wrapper class should get an "update()"
method.

Python 2.5 doesn't have abc module, and both Python 2.5 and Python 3.1 don't work with recent Cython.
@kmike
Copy link
Collaborator Author

kmike commented Nov 2, 2014

At first I haven't used abc because for some reason I though it is not available in Python 2.6. It is a bit slower with collections.Mapping, maybe because the iteration code is less efficient.

In [1]: from lupa import LuaRuntime
In [2]: lua = LuaRuntime()
In [3]: obj = {str(k):k for k in range(1000)}
In [4]: %timeit tbl = lua.table_from(obj)  # with dicts
10000 loops, best of 3: 129 µs per loop
In [4]: %timeit tbl = lua.table_from(obj)  # with collections.Mapping
10000 loops, best of 3: 134 µs per loop

I've removed **kwargs so that we can add recursive=True argument in future. Recursive conversion needs infinite recursion protection (max_depth?); it could also make sense to move this logic to py_to_lua function, but I'm not sure. Could we handle this all later?

@kmike
Copy link
Collaborator Author

kmike commented Nov 2, 2014

I have troubles with wrapped Lua tables; table.items() segfaults when used in table_from(), so registering _LuaTable as collections.Mapping doesn't work.

@scoder
Copy link
Owner

scoder commented Nov 2, 2014

Ah, yes, I can imaging that it gets the Lua stack corrupted when both table_from() and table.items() try to use it at the same time. That means we really have to special case Lua tables and copy the values to the new table explicitly. There should be a quicker way to do that than going through LuaIter anyway, e.g.

https://stackoverflow.com/questions/4535152/cloning-a-lua-table-in-lua-c-api

@kmike
Copy link
Collaborator Author

kmike commented Nov 2, 2014

I've added support for _LuaTable, but not for _LuaIter.

@kmike
Copy link
Collaborator Author

kmike commented Nov 2, 2014

There is some protection for _LuaIter, but lua.table_from(k for k in other_table.keys()) still segfaults.

@scoder
Copy link
Owner

scoder commented Nov 2, 2014

Looks good (enough) to me, I'll merge it. Fixing support for LuaIter requires some cleanup in various places that currently assume that clearing the stack is sufficient when they are done with their work. Instead, they should pop the right number of elements to properly support nesting.

@scoder
Copy link
Owner

scoder commented Nov 2, 2014

Thanks for your work.

scoder added a commit that referenced this pull request Nov 2, 2014
LuaRuntime.table_from method
@scoder scoder merged commit 03b38ff into scoder:master Nov 2, 2014
@scoder scoder changed the title [WIP] LuaRuntime.table_from method LuaRuntime.table_from method Nov 2, 2014
@kmike
Copy link
Collaborator Author

kmike commented Nov 2, 2014

Thanks for a great wrapper!

Lua C API is quite interesting to learn, I haven't used stack-based language APIs before.

@scoder
Copy link
Owner

scoder commented Nov 2, 2014

I've fixed most places but not all of them here: bf1c644

The remaining ones need some thought, especially the function calls, where the number of elements on the stack is the actual number of arguments and keeping things around on the stack will screw it up.

@kmike
Copy link
Collaborator Author

kmike commented Nov 2, 2014

Great, it seems this made special-casing of _LuaIter unnecessary: #40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants