Re: [Tarantool-patches] [PATCH] test: fix warning for old gcc at testgetmetrics.c

[Igor Munkin <imun@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