From: Sergey Kaplun <skaplun@tarantool.org> To: Igor Munkin <imun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH 1/2] test: force enable assert checks in release build Date: Thu, 15 Oct 2020 10:23:34 +0300 [thread overview] Message-ID: <20201015072334.GB9302@root> (raw) In-Reply-To: <20201014192935.GH18920@tarantool.org> Hi, Igor! Thanks for the review! On 14.10.20, Igor Munkin wrote: > Sergey, > > Thanks for the patch! Consider a couple nits below. > > On 14.10.20, Sergey Kaplun wrote: > > This patch fixes the regression introduced in scope of > > 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API for > > platform metrics'). As a result of the patch release build > > was failed according to -Werror compiler flag and unused variables that > > used only for assertions checks. > > > > Force #undef NDEBUG directive leaves asserts on and allows not disable > > Minor: IMHO "leaves asserts enabled, so the introduced tests works even > with release build" is a little better, but this doesn't change the > meaning, so feel free to ignore. I've updated commit message and branch with that. > > > newly added tests for release build. > > > > Follows up tarantool/tarantool#5187 > > --- > > test/misclib-getmetrics-capi/testgetmetrics.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c > > index 8844b17..3b6f599 100644 > > --- a/test/misclib-getmetrics-capi/testgetmetrics.c > > +++ b/test/misclib-getmetrics-capi/testgetmetrics.c > > @@ -4,6 +4,7 @@ > > > > #include <lmisclib.h> > > > > +#undef NDEBUG > > Minor: It would be nice to drop a few words regarding this hack. At the > same time, the code is well-structured, so it's quite obvious that you > want asserts to be always enabled. Feel free to ignore. I've Added tiny comment to make changes obvious. See iterative patch below. Branch is force pushed. > > > #include <assert.h> > > > > static int base(lua_State *L) > > -- > > 2.28.0 > > > > -- > Best regards, > IM =================================================================== diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c index 3b6f599..768c1a9 100644 --- a/test/misclib-getmetrics-capi/testgetmetrics.c +++ b/test/misclib-getmetrics-capi/testgetmetrics.c @@ -4,6 +4,7 @@ #include <lmisclib.h> +/* Force enable asserts for release build. */ #undef NDEBUG #include <assert.h> =================================================================== -- Best regards, Sergey Kaplun
next prev parent reply other threads:[~2020-10-15 7:24 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-14 17:03 [Tarantool-patches] [PATCH 0/2] Fix broken CI after introduced LuaJIT metrics Sergey Kaplun 2020-10-14 17:03 ` [Tarantool-patches] [PATCH 1/2] test: force enable assert checks in release build Sergey Kaplun 2020-10-14 19:29 ` Igor Munkin 2020-10-14 19:33 ` Igor Munkin 2020-10-15 7:23 ` Sergey Kaplun [this message] 2020-10-15 9:02 ` Sergey Ostanevich 2020-10-14 17:03 ` [Tarantool-patches] [PATCH 2/2] test: disable jit related tests at FreeBSD Sergey Kaplun 2020-10-14 19:43 ` Igor Munkin 2020-10-15 7:25 ` Sergey Kaplun 2020-10-15 9:04 ` Sergey Ostanevich 2020-10-14 19:47 ` [Tarantool-patches] [PATCH 0/2] Fix broken CI after introduced LuaJIT metrics Igor Munkin 2020-10-15 9:22 ` Kirill Yukhin
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=20201015072334.GB9302@root \ --to=skaplun@tarantool.org \ --cc=imun@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 1/2] test: force enable assert checks in release build' \ /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