[Tarantool-patches] [PATCH 2/3] lua: table.deepcopy ignores __pairs metamethod
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Feb 14 01:50:17 MSK 2020
Thanks for the patch!
See 5 comments below.
1. Commit title uses narration, while it should use imperative.
For example, "lua: don't use pairs() in table.deepcopy".
On 13/02/2020 21:33, olegrok at tarantool.org wrote:
> From: Oleg Babin <babinoleg at mail.ru>
>
> After 1d85144a9b4bbbb026402848efde1ab98bf72633
2. After commit hash please put its title in ().
> table.deepcopy changed the behaviour and started
> iterate through tables considering __pairs metamethod.
> In some cases it broke backward compatibility.
> To avoid such problem let's ignore __pairs
> and iterate through tables as it was before
>
> Closes #4770
3. Unfortunately, things are not that simple. This patch
does not really close the issue, because pairs(space_object)
is still broken. But your commit probably is useful anyway in
case we will decide to keep 5.2 __pairs/__ipairs. And it would
unblock vshard development.
Summary: just use 'Part of #4770' instead of 'Closes'.
> Follow-up #4560
> ---
> Issue: https://github.com/tarantool/tarantool/issues/4770
> Branch: https://github.com/tarantool/tarantool/tree/olegrok/table-fixes
> src/lua/table.lua | 7 ++++++-
> test/app-tap/table.test.lua | 26 +++++++++++++++++++++++++-
> 2 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/src/lua/table.lua b/src/lua/table.lua
> index d83217dcb..4fa9c421c 100644
> --- a/src/lua/table.lua
> +++ b/src/lua/table.lua
> @@ -1,3 +1,8 @@
> +-- This pairs implementation doesn't trigger __pairs metamethod
> +local function internal_pairs(tbl)
> + return next, tbl, nil
> +end
4. I would call it 'rawpairs', by analogue with 'rawset/get/*'
functions.
> +
> local function table_deepcopy_internal(orig, cyclic)
> cyclic = cyclic or {}
> local copy = orig
> @@ -10,7 +15,7 @@ local function table_deepcopy_internal(orig, cyclic)
> copy = cyclic[orig]
> else
> cyclic[orig] = copy
> - for orig_key, orig_value in pairs(orig) do
> + for orig_key, orig_value in internal_pairs(orig) do
> local key = table_deepcopy_internal(orig_key, cyclic)
> copy[key] = table_deepcopy_internal(orig_value, cyclic)
> end
> diff --git a/test/app-tap/table.test.lua b/test/app-tap/table.test.lua
> index 07894f69e..3faf2ed23 100755
> --- a/test/app-tap/table.test.lua
> +++ b/test/app-tap/table.test.lua
> @@ -241,4 +241,28 @@ do -- gh-4340: deepcopy doesn't handle __metatable correctly.
> )
> end
>
> +do -- gh-4770: deepcopy uses __pairs for iteration over table.
> + local original = { a = 1, b = 2 }
> +
> + local function custom_pairs(self)
> + local function step(tbl, k)
> + local k, v = next(tbl, k)
> + if v ~= nil then
> + v = v + 1
> + end
> + return k, v
> + end
> + return step, self, nil
5. I propose you to just add 'assert(false)' as a body of
custom pairs. To ensure that it is never called by
deepcopy. Because anyway it is not called now in your
test.
> + end
> +
> + setmetatable(original, {__pairs = custom_pairs })
> +
> + -- Don't use is deeply as it could use pairs for check
> + local copy = table.deepcopy(original)
> + test:is(original.a, copy.a,
> + "checking that the first values is correctly copied")
> + test:is(original.b, copy.b,
> + "checking that the second values is correctly copied")
> +end
> +
> os.exit(test:check() == true and 0 or 1)
>
More information about the Tarantool-patches
mailing list