[Tarantool-patches] [PATCH luajit] LJ_GC64: Fix lua_concat().
Sergey Bronnikov
sergeyb at tarantool.org
Tue Oct 3 18:35:15 MSK 2023
Hi, Max
thanks for review, see my comments below
Sergey
On 9/29/23 11:24, Maxim Kokryashkin via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> On Tue, Sep 26, 2023 at 09:56:31AM +0300, Sergey Bronnikov via Tarantool-patches wrote:
>> From: Sergey Bronnikov <sergeyb at tarantool.org>
>>
>> Reported by Mathias Westerdahl.
>>
>> (cherry picked from commit 633f265f67f322cbe2c5fd11d3e46d968ac220f7)
>>
>> Lua 5.1 Reference Manual [1] defines a function `lua_concat`, that:
>>
>>> void lua_concat (lua_State *L, int n);
>>>
>>> Concatenates the n values at the top of the stack, pops them, and leaves
>>> the result at the top.
>> Without the patch `lua_concat()` behaved incorrectly with userdata with
>> defined `__concat` metamethod. The problem is GC64-specific.
>>
>> Assuming we have three literals and a userdata with defined "__concat"
>> metamethod on top of the Lua stack:
>>
>> 1 [string]
>> 2 [string]
>> 3 [string]
>> 4 [string]
>> 5 [userdata] <--- top
>>
>> On attempt to concatenate *two* items on top Lua stack, `lua_concat()`
> Typo: s/on top/on top of the/
Fixed.
>> concatenates *four* items and leaves result at the top:
> Typo: s/at the/on the/
Fixed.
>> 1 [string]
>> 2 [string][string][string][userdata] <--- top
>>
>> The problem is in incorrect calculation of `n` counter in a loop in
> Typo: s/in a loop/in the loop/
Fixed.
>> implementation of function `lua_concat`. Without a fix `n` is equal to 3
> Typo: s/a fix/the fix/
Fixed.
>> at the end of the first iteration and therefore it goes to the next
>> iteration of concatenation. In a fixed implementation of `lua_concat()`
> Typo: s/In a fixed/In the fixed/
Fixed.
>> `n` is equal to 1 at the end of the first loop iteration, decremented in
>> a loop postcondition and breaks a loop.
> Typo: s/a loop/the loop/
Fixed.
<snipped> because no errors, typos, objections were found
>> +
>> +static const luaL_Reg INFO_methods[] =
>> +{
>> + {0, 0}
> Nit: It is more common to write `NULL` as sentinel.
Fixed.
>> +};
> This table is redundant.
Fixed.
>> +
>> +static const luaL_Reg INFO_meta[] =
>> +{
>> + {"__concat", INFO_concat},
>> + {0, 0}
> Nit: It is more common to write `NULL` as sentinel.
Fixed too.
>> +};
>> +
>> +static int lua_concat_testcase(void *test_state)
>> +{
>> + lua_State *L = test_state;
>> +
>> + /* Create methods table, add it to the globals. */
>> + luaL_register(L, TYPE_NAME_INFO, INFO_methods);
>> + int methods_idx = lua_gettop(L);
> The methods table is redundant.
Fixed as well.
>> + /* Create metatable and add it to the Lua registry. */
>> + luaL_newmetatable(L, TYPE_NAME_INFO);
>> +
>> + int metatable_idx = lua_gettop(L);
>> + /* Fill metatable. */
>> + luaL_register(L, 0, INFO_meta);
> The middle argument is meant to be NULL.
Sure, fixed.
>> +
>> + lua_pushliteral(L, "__metatable");
>> + /* Duplicate methods table. */
>> + lua_pushvalue(L, methods_idx);
>> + lua_settable(L, metatable_idx);
>> + lua_pop(L, 2);
> This whole part can be reduced to this:
> | /* Create metatable and add it to the Lua registry. */
> | luaL_newmetatable(L, TYPE_NAME_INFO);
> |
> | /* Fill metatable. */
> | luaL_register(L, 0, INFO_meta);
> | lua_pop(L, 1);
> |
Reduced, thanks.
>
>> +
>> + assert_true(lua_gettop(L) == 0);
>> +
>> + Info info;
>> + info.value = 7;
>> +
>> + lua_pushliteral(L, "C");
>> + lua_pushliteral(L, "B");
>> + lua_pushliteral(L, "A");
>> + lua_pushinfo(L, &info);
>> +
>> + int top = 4;
>> + assert_true(lua_gettop(L) == top);
>> +
>> + /**
>> + * void lua_concat (lua_State *L, int n);
>> + *
>> + * Concatenates the n values at the top of the stack,
>> + * pops them, and leaves the result at the top. If n is 1,
>> + * the result is the single value on the stack; if n is 0,
>> + * the result is the empty string [1].
>> + *
>> + * 1. https://www.lua.org/manual/5.1/manual.html
>> + */
>> + int n = 2;
>> + lua_concat(L, n);
>> +
>> + const char *str = lua_tostring(L, -1);
>> + assert_true(top - n + 1 == lua_gettop(L));
> We have `assert_int_equal` in `test.h` for that.
Sure, added in a separate patch and updated branch force-pushed.
commit 71f4161115c2e2c13c91e2804f9c41f673ada5ce
Author: Sergey Bronnikov <sergeyb at tarantool.org>
Date: Tue Oct 3 17:39:48 2023 +0300
test: introduce asserts assert_str{_not}_equal
The patch follows up commit a0483bd214f2 ("test: introduce module for C
tests") and adds additional asserts suitable for comparing strings.
diff --git a/test/tarantool-c-tests/README.md
b/test/tarantool-c-tests/README.md
index 462534be..8fad6407 100644
--- a/test/tarantool-c-tests/README.md
+++ b/test/tarantool-c-tests/README.md
@@ -35,6 +35,8 @@ glibc `assert()`:
equal to the `b`.
* `assert_double{_not}_equal(a, b)` -- check that two doubles are {not}
**exactly** equal.
+* `assert_str{_not}_equal(a, b)` -- check that `char *` variable `a` is
{not}
+ equal to the `b`.
## Directives
diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
index 8b14c705..bbf573b2 100644
--- a/test/tarantool-c-tests/test.h
+++ b/test/tarantool-c-tests/test.h
@@ -13,8 +13,6 @@
* * Helpers assert macros:
* - assert_uint_equal if needed
* - assert_uint_not_equal if needed
- * - assert_str_equal if needed
- * - assert_str_not_equal if needed
* - assert_memory_equal if needed
* - assert_memory_not_equal if needed
* * Pragmas.
@@ -214,4 +212,19 @@ static inline int todo(const char *reason)
); \
} while (0)
+#define assert_str_equal(got, expected, n) do { \
+ assert_general(strncmp(got, expected, n) == 0, \
+ ASSERT_EQUAL_FMT(int, "%d"), \
+ __FILE__, __LINE__, (got), (expected) \
+ ); \
+} while (0)
+
+#define assert_str_not_equal(got, unexpected, n) do { \
+ assert_general(strncmp(got, expected, n) != 0, \
+ ASSERT_NOT_EQUAL_FMT(int, "%d"), \
+ __FILE__, __LINE__, (got), (unexpected) \
+ ); \
+} while (0)
+
+
#endif /* TARANTOOL_LUAJIT_TEST_H */
>> + assert_true(strcmp(str, "A[Info.value=7]") == 0);
> One should never use any of the unrestricted string functions, as
> they are unsafe. Always use the `n`-limited versions: `strncmp`
> instead of `strcmp`, `strnlen` instead of `strlen`, etc.
>
> Side note: there is a TODO in test.h about `assert_str_equal`,
> maybe it's a great time to introduce it in this patch.
Ok, replaced with strncmp.
>
>> +
>> + /* Cleanup. */
>> + lua_settop(L, 0);
>> +
>> + return TEST_EXIT_SUCCESS;
>> +}
> Some general comments about that function:
> 1. Please mark variables as `const` where possible.
Done.
> 2. C style requires you to put variable declarations at the
> top of the function.
Done.
> 3. You have three distinct parts in this function: set up procedure,
> the test itself, and the the tear down. It would be better to split them
> into three separate functions to improve readability.
Added a comments for splitting sections visually.
>> +
>> +int main(void)
>> +{
>> + lua_State *L = utils_lua_init();
>> + const struct test_unit tgroup[] = {
>> + test_unit_def(lua_concat_testcase),
>> + };
>> + const int test_result = test_run_group(tgroup, L);
>> + utils_lua_close(L);
>> +
>> + return test_result;
>> +}
>> --
>> 2.34.1
>>
> Best regards,
> Maxim Kokryashkin
More information about the Tarantool-patches
mailing list