From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp10.mail.ru (smtp10.mail.ru [94.100.181.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 45150469719 for ; Thu, 15 Oct 2020 12:02:43 +0300 (MSK) Date: Thu, 15 Oct 2020 12:02:42 +0300 From: Sergey Ostanevich Message-ID: <20201015090242.GF2885@tarantool.org> References: <20201014192935.GH18920@tarantool.org> <20201015072334.GB9302@root> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20201015072334.GB9302@root> Subject: Re: [Tarantool-patches] [PATCH 1/2] test: force enable assert checks in release build List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! LGTM. Sergos. On 15 окт 10:23, Sergey Kaplun wrote: > 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 > > > > > > +#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 > > > > > > 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 > > +/* Force enable asserts for release build. */ > #undef NDEBUG > #include > > =================================================================== > > -- > Best regards, > Sergey Kaplun