From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Maxim Kokryashkin <m.kokryashkin@tarantool.org>, Sergey Bronnikov <estetus@gmail.com> Cc: max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix lua_concat(). Date: Tue, 3 Oct 2023 18:35:15 +0300 [thread overview] Message-ID: <2e683729-401b-1b86-5a60-54a2cd39dc84@tarantool.org> (raw) In-Reply-To: <fyefr5qmah2mxwkbfa2fphnnml4pktrblzq7arpqmn2rj3v4k6@hwmhdcvj7cue> 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@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@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
next prev parent reply other threads:[~2023-10-03 15:35 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <cover.1698775628.git.sergeyb@tarantool.org> 2023-09-26 6:56 ` Sergey Bronnikov via Tarantool-patches 2023-09-29 8:24 ` Maxim Kokryashkin via Tarantool-patches 2023-10-03 15:35 ` Sergey Bronnikov via Tarantool-patches [this message] 2023-10-04 10:48 ` Maxim Kokryashkin via Tarantool-patches 2023-10-06 13:51 ` Sergey Bronnikov via Tarantool-patches 2023-10-31 18:11 ` [Tarantool-patches] [PATCH luajit 1/2] test: introduce asserts assert_str{_not}_equal Sergey Bronnikov via Tarantool-patches 2023-11-01 7:40 ` Sergey Kaplun via Tarantool-patches 2023-11-01 8:28 ` Igor Munkin via Tarantool-patches 2023-11-10 11:41 ` Sergey Bronnikov via Tarantool-patches 2023-11-14 8:55 ` Sergey Kaplun via Tarantool-patches 2023-11-15 9:32 ` Sergey Bronnikov via Tarantool-patches 2023-11-16 8:02 ` Sergey Kaplun via Tarantool-patches 2023-11-18 16:40 ` Sergey Bronnikov via Tarantool-patches 2023-11-20 9:28 ` Sergey Kaplun via Tarantool-patches 2023-11-20 13:19 ` Igor Munkin via Tarantool-patches 2023-11-10 11:40 ` Sergey Bronnikov via Tarantool-patches 2023-11-23 6:31 ` [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix lua_concat() Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=2e683729-401b-1b86-5a60-54a2cd39dc84@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=estetus@gmail.com \ --cc=m.kokryashkin@tarantool.org \ --cc=max.kokryashkin@gmail.com \ --cc=sergeyb@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix lua_concat().' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox