From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (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 81573469719 for ; Thu, 15 Oct 2020 10:24:01 +0300 (MSK) Date: Thu, 15 Oct 2020 10:23:34 +0300 From: Sergey Kaplun Message-ID: <20201015072334.GB9302@root> References: <20201014192935.GH18920@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201014192935.GH18920@tarantool.org> 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: Igor Munkin Cc: tarantool-patches@dev.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 > > > > +#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