From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 5D82D469719 for ; Tue, 20 Oct 2020 14:20:20 +0300 (MSK) Date: Tue, 20 Oct 2020 14:06:00 +0300 From: Igor Munkin Message-ID: <20201020110600.GF5396@tarantool.org> References: <20201017114459.18598-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20201017114459.18598-1-skaplun@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] test: fix warning for old gcc at testgetmetrics.c List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Sergey, Thanks for the patch! The patch LGTM, but there are several nits below, please consider them. On 17.10.20, Sergey Kaplun wrote: > This patch fixes the regression introduced in scope of > 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua > API for platform metrics'). > > Old gcc compiler (gcc-4.8.5-39) throws warnings like: > "error: comparison is always true due to limited range of data type > [-Werror=type-limits]", when you cast syze_t value to ssyze_t type and Typo: s/syze_t/size_t/g. > compare its to >= 0. This leads to failing compilation with -Werror > flag. > > Taking into account that this base test check only API format, not Typo: s/check/checks/. > values by themself, it will be enough to use (void) construction here. > > Follows up tarantool/tarantool#5187 > --- > Bug was reported by Sasha Tikhonov. He faced it up when test his own > branch (avtikhon/gh-4941-gcc-old) with enabled default gcc at Centos 7. > There was failure like [1]: > =================================================================== > /build/usr/src/debug/tarantool-2.6.0.208/third_party/luajit/test/misclib-getmetrics-capi/testgetmetrics.c:41:9: error: comparison is always true due to limited range of data type [-Werror=type-limits] > assert((ssize_t)metrics.jit_trace_num >= 0); > ^ > =================================================================== > > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5187-old-gcc-warning > > Branch for test CI was rebased on top of avtikhon/gh-4941-gcc-old which > enables CI-checks with default gcc on CentOS 7 > > CI is still red but now there are tons of warnings like [2]: > =================================================================== > /usr/include/features.h:330:4: error: #warning _FORTIFY_SOURCE requires compiling with optimization (-O) [-Werror=cpp] > # warning _FORTIFY_SOURCE requires compiling with optimization (-O) Side note: this is faced here, this is also faced while package building. I hope this will tire someone and one will finally fix this crap in our CMake recipes. > =================================================================== > or replication|txn_manager (see also [3]) tests failures not related to > LuaJIT. > > Side note: This is old pipeline, but I changed only comment format > before repushed branch. > > CI: https://gitlab.com/tarantool/tarantool/-/pipelines/203899226 > > [1]: https://gitlab.com/tarantool/tarantool/-/jobs/795394423#L1999 > [2]: https://gitlab.com/tarantool/tarantool/-/jobs/796433283#L1637 > [3]: https://github.com/tarantool/tarantool/issues/5423 > > test/misclib-getmetrics-capi/testgetmetrics.c | 51 +++++++++---------- > 1 file changed, 24 insertions(+), 27 deletions(-) > > diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c > index 3b6f599..fc19680 100644 > --- a/test/misclib-getmetrics-capi/testgetmetrics.c > +++ b/test/misclib-getmetrics-capi/testgetmetrics.c > @@ -12,33 +12,30 @@ static int base(lua_State *L) > struct luam_Metrics metrics; > luaM_metrics(L, &metrics); > > - /* > - * Just check API. > - * (ssize_t) cast need for not always-true unsigned >= 0 check. > - */ > - assert((ssize_t)metrics.strhash_hit >= 0); > - assert((ssize_t)metrics.strhash_miss >= 0); > - > - assert((ssize_t)metrics.gc_strnum >= 0); > - assert((ssize_t)metrics.gc_tabnum >= 0); > - assert((ssize_t)metrics.gc_udatanum >= 0); > - assert((ssize_t)metrics.gc_cdatanum >= 0); > - > - assert((ssize_t)metrics.gc_total >= 0); > - assert((ssize_t)metrics.gc_freed >= 0); > - assert((ssize_t)metrics.gc_allocated >= 0); > - > - assert((ssize_t)metrics.gc_steps_pause >= 0); > - assert((ssize_t)metrics.gc_steps_propagate >= 0); > - assert((ssize_t)metrics.gc_steps_atomic >= 0); > - assert((ssize_t)metrics.gc_steps_sweepstring >= 0); > - assert((ssize_t)metrics.gc_steps_sweep >= 0); > - assert((ssize_t)metrics.gc_steps_finalize >= 0); > - > - assert((ssize_t)metrics.jit_snap_restore >= 0); > - assert((ssize_t)metrics.jit_trace_abort >= 0); > - assert((ssize_t)metrics.jit_mcode_size >= 0); > - assert((ssize_t)metrics.jit_trace_num >= 0); > + /* Just check API, not values by itself. */ Minor: It looks like only structure fields check (i.e. interface, contract), but not the values they contain (i.e. API), so I propose to adjust the wording. > + (void)metrics.strhash_hit; > + (void)metrics.strhash_miss; > + > + (void)metrics.gc_strnum; > + (void)metrics.gc_tabnum; > + (void)metrics.gc_udatanum; > + (void)metrics.gc_cdatanum; > + > + (void)metrics.gc_total; > + (void)metrics.gc_freed; > + (void)metrics.gc_allocated; > + > + (void)metrics.gc_steps_pause; > + (void)metrics.gc_steps_propagate; > + (void)metrics.gc_steps_atomic; > + (void)metrics.gc_steps_sweepstring; > + (void)metrics.gc_steps_sweep; > + (void)metrics.gc_steps_finalize; > + > + (void)metrics.jit_snap_restore; > + (void)metrics.jit_trace_abort; > + (void)metrics.jit_mcode_size; > + (void)metrics.jit_trace_num; > > lua_pushboolean(L, 1); > return 1; > -- > 2.28.0 > -- Best regards, IM