From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id F1EBAF5DAC0; Wed, 5 Mar 2025 10:55:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org F1EBAF5DAC0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1741161340; bh=10vwbmSbnyJ8qSI7hlXy+XijPnkLfihHROIHSvJfpG0=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=MgmeWcsRowijlGI00a2UrkgIekdtYgy89xe2kWBsOwk3xEm4GknuFcEuGn1JZb6XR sk0zacQ+szbHMY5oV2rJcQKivpU7G0HT+LUciPrSNweIQcQK6+BQDAOni8X+YQeC60 gYD/7Efn1IQ/VLx0fwJj5AnLtabeOi56AatO6AYM= Received: from send129.i.mail.ru (send129.i.mail.ru [89.221.237.224]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 897C5F5DAC0 for ; Wed, 5 Mar 2025 10:55:38 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 897C5F5DAC0 Received: by exim-smtp-848fbd8fb5-2c8v6 with esmtpa (envelope-from ) id 1tpjbN-00000000H5B-15sG; Wed, 05 Mar 2025 10:55:37 +0300 Date: Wed, 5 Mar 2025 10:55:37 +0300 To: Sergey Bronnikov Cc: Sergey Bronnikov , tarantool-patches@dev.tarantool.org Message-ID: References: <881980705ead69f58c0474d5f49c00603e4c6e72.1740050074.git.sergeyb@tarantool.org> <52e2b52f-33a8-4f06-bcd8-ec8aa9fc457a@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <52e2b52f-33a8-4f06-bcd8-ec8aa9fc457a@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9A2A28D6C22EE4403F72BE999420CB4FCA66B09A771679EDC182A05F5380850407DB75DC16F2A74263DE06ABAFEAF67055D45265AD82EADCA72CD49F772FD6A01A12C60E53AB0ACD6 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A3DED2DACB82E709C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6759CC434672EE6371C2A783ECEC0211ADC4224003CC836476D5A39DEEDB180909611E41BBFE2FEB2BE5B952C75F7F857E11DB69C538B952F92C4D3382E2E5A7EFBC0CAF8F1363D5DE9FA2833FD35BB23D9E625A9149C048EE9ECD01F8117BC8BEA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18F04B652EEC242312D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE0AC5B80A05675ACD4D0DA9BD313A0613D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE3F8BD4E506CFA3D882D242C3BD2E3F4C6C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637B8F435DEDE9E76EBEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A531BEB7B0D3197F665002B1117B3ED69637C0C276ECDA482A219207EC0A953D2C823CB91A9FED034534781492E4B8EEADB05233B9BC4759D3 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFAA7CDE47F2F373BAAFE8858F21DB2143F5F6E651C5C4402BA450B2A7ABD5D602192AF36DABE858452C10F42E4F22A8B6AAD00238EF3FFA651442C283943A9CD6347F071AD8A23DA95F4332CA8FE04980913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVQiWK+2I7Y2s/iVkUzgetEw= X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B5979CEDC54EDA8EE303DE06ABAFEAF67055D45265AD82EADCAB7CBEF92542CD7C88B0A2698F12F5C9EC77752E0C033A69E86920BD37369036789A8C6A0E60D2BB63A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 4/8][v3] sysprof: introduce specific errors and default mode X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the fixes! LGTM, after fixing 3 minor nits below. On 24.02.25, Sergey Bronnikov wrote: > Hi, Sergey, > > changes applied and force-pushed. > > Sergey > > On 24.02.2025 15:46, Sergey Kaplun via Tarantool-patches wrote: > > Hi, Sergey! > > Thanks for the fixes! > > LGTM, after the final polishing, please consider my comments below. > > > > On 20.02.25, Sergey Bronnikov wrote: > >> --- > >> src/lib_misc.c | 77 ++++++++++++------- > >> src/lj_errmsg.h | 5 ++ > >> .../profilers/misclib-sysprof-lapi.test.lua | 68 ++++++++++++++-- > >> 3 files changed, 114 insertions(+), 36 deletions(-) > >> > >> diff --git a/src/lib_misc.c b/src/lib_misc.c > >> index 5b7a4b62..1fd06dd1 100644 > >> --- a/src/lib_misc.c > >> +++ b/src/lib_misc.c > >> @@ -14,6 +14,7 @@ > >> > >> #include "lj_obj.h" > >> #include "lj_str.h" > >> +#include "lj_strfmt.h" > > Please add the corresponding include (after lj_str.h) in to > > avoid conflicts when we use this file (for crossbuilds or whatever). There is no corresponding update on the branch. > > > >> -static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, int idx) { > >> - GCtab *options = lj_lib_checktab(L, idx); > >> +static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, > >> + const char **err_details) { > > Nit: Please, replace every 8 spaces with one tab. > > Fixed: > > --- a/src/lib_misc.c > +++ b/src/lib_misc.c > @@ -180,7 +180,7 @@ static int set_output_path(const char *path, struct > luam_Sysprof_Options *opt) { >  } > >  static int parse_sysprof_opts(lua_State *L, struct > luam_Sysprof_Options *opt, > -                             const char **err_details) { > +                             const char **err_details) { Thanks! Please apply the following patch. It fixes all patchset-related places. =================================================================== diff --git a/src/lib_misc.c b/src/lib_misc.c index 94ec6564..79cfaf7b 100644 --- a/src/lib_misc.c +++ b/src/lib_misc.c @@ -199,13 +199,13 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, cTValue *mode_opt = lj_tab_getstr(options, lj_str_newlit(L, "mode")); if (mode_opt) { if (!tvisstr(mode_opt)) { - *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE); - return PROFILE_ERRUSE; + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE); + return PROFILE_ERRUSE; } mode = strVdata(mode_opt); if (strlen(mode) == 0 || mode[1] != '\0') { - *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE); - return PROFILE_ERRUSE; + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE); + return PROFILE_ERRUSE; } } @@ -223,8 +223,8 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, opt->mode = LUAM_SYSPROF_CALLGRAPH; break; default: - *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE); - return PROFILE_ERRUSE; + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE); + return PROFILE_ERRUSE; } } @@ -235,8 +235,8 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, if (interval && tvisnumber(interval)) { int32_t signed_interval = numberVint(interval); if (signed_interval < 1) { - *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADINTERVAL); - return PROFILE_ERRUSE; + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADINTERVAL); + return PROFILE_ERRUSE; } opt->interval = signed_interval; } @@ -281,9 +281,9 @@ static int sysprof_error(lua_State *L, int status, const char *err_details) case PROFILE_ERRUSE: lua_pushnil(L); if (err_details) - lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details); + lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details); else - lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE)); + lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE)); lua_pushinteger(L, EINVAL); return 3; #if LJ_HASSYSPROF =================================================================== >    int n = (int)(L->top - L->base); > >> +test:is(res, false, "res with not a table") > >> +test:ok(err:match("table expected, got string"), > >> + "error with not a table") > >> +test:is(errno, nil, "errno with not a table") > > There is no need to check or declare the `errno` variable. It's always > > `nil` for the case when the error is raised. > > The check is cheap, why not? This is kind of misleading, plus this is a dead check. It is __always__ `nil`, or we will get an error in the first test and the second test. Since we are not testing `pcall()` implementation here, let's just drop it (it is even cheaper :)). -- Best regards, Sergey Kaplun