<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hi, Sergey!</p>
<p>Changes applied and force-pushed.</p>
<p>Sergey<br>
</p>
<div class="moz-cite-prefix">On 05.03.2025 10:55, Sergey Kaplun
wrote:<br>
</div>
<blockquote type="cite" cite="mid:Z8gDeRZfzqpt0jj_@root">
<pre wrap="" class="moz-quote-pre">Hi, Sergey!
Thanks for the fixes!
LGTM, after fixing 3 minor nits below.
On 24.02.25, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">Hi, Sergey,
changes applied and force-pushed.
Sergey
On 24.02.2025 15:46, Sergey Kaplun via Tarantool-patches wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">Hi, Sergey!
Thanks for the fixes!
LGTM, after the final polishing, please consider my comments below.
On 20.02.25, Sergey Bronnikov wrote:
</pre>
</blockquote>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">---
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"
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">Please add the corresponding include (after lj_str.h) in <src/Makefile.dep.original> to
avoid conflicts when we use this file (for crossbuilds or whatever).
</pre>
</blockquote>
</blockquote>
<pre wrap="" class="moz-quote-pre">
There is no corresponding update on the branch.</pre>
</blockquote>
<p>Sorry, missed this comment.</p>
<p>Fixed:</p>
<p>--- a/src/Makefile.dep.original<br>
+++ b/src/Makefile.dep.original<br>
@@ -30,7 +30,7 @@ lib_jit.o: lib_jit.c lua.h luaconf.h lauxlib.h
lualib.h lj_obj.h lj_def.h \<br>
lib_math.o: lib_math.c lua.h luaconf.h lauxlib.h lualib.h
lj_obj.h \<br>
lj_def.h lj_arch.h lj_lib.h lj_vm.h lj_libdef.h<br>
lib_misc.o: lib_misc.c lua.h luaconf.h lmisclib.h lauxlib.h
lj_obj.h \<br>
- lj_def.h lj_arch.h lj_str.h lj_tab.h lj_lib.h lj_gc.h lj_err.h \<br>
+ lj_def.h lj_arch.h lj_str.h lj_strfmt.h lj_tab.h lj_lib.h
lj_gc.h lj_err.h \<br>
lj_errmsg.h lj_memprof.h lj_wbuf.h lj_libdef.h<br>
lib_os.o: lib_os.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h
lj_def.h \<br>
lj_arch.h lj_gc.h lj_err.h lj_errmsg.h lj_buf.h lj_str.h
lj_lib.h \<br>
</p>
<blockquote type="cite" cite="mid:Z8gDeRZfzqpt0jj_@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
</pre>
</blockquote>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">-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) {
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">Nit: Please, replace every 8 spaces with one tab.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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) {
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Thanks!
Please apply the following patch. It fixes all patchset-related places.</pre>
</blockquote>
Applied.<br>
<blockquote type="cite" cite="mid:Z8gDeRZfzqpt0jj_@root">
<pre wrap="" class="moz-quote-pre">
===================================================================
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
===================================================================
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> int n = (int)(L->top - L->base);
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+test:is(res, false, "res with not a table")
+test:ok(<a class="moz-txt-link-freetext" href="err:match(">err:match(</a>"table expected, got string"),
+ "error with not a table")
+test:is(errno, nil, "errno with not a table")
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">There is no need to check or declare the `errno` variable. It's always
`nil` for the case when the error is raised.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
The check is cheap, why not?
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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 :)).</pre>
</blockquote>
<p>okay, removed:</p>
<p>@@ -85,11 +85,10 @@ <a class="moz-txt-link-freetext" href="test:ok(ok">test:ok(ok</a> == false and err_<a class="moz-txt-link-freetext" href="msg:match(">msg:match(</a>"cannot
open a file"),<br>
assert(misc.sysprof.stop())<br>
<br>
-- Not a table.<br>
-res, err, errno = pcall(misc.sysprof.start, "NOT A TABLE")<br>
+res, err = pcall(misc.sysprof.start, "NOT A TABLE")<br>
<a class="moz-txt-link-freetext" href="test:is(res">test:is(res</a>, false, "res with not a table")<br>
<a class="moz-txt-link-freetext" href="test:ok(err:match(">test:ok(err:match(</a>"table expected, got string"),<br>
"error with not a table")<br>
-test:is(errno, nil, "errno with not a table")<br>
<br>
-- All parameters are invalid. Check parameter validation order<br>
-- (not strict documented behaviour).<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:Z8gDeRZfzqpt0jj_@root">
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>