[tarantool-patches] Re: [PATCH] Fix warnings.

[Alexander Turenko <alexander.turenko@tarantool.org>]

Hi, Gleb!

Consider comments below.

I propose to add another release target with -Werror, consider [g].

[g]: https://github.com/tarantool/tarantool/issues/3238#issuecomment-377319953

WBR, Alexander Turenko.

On Wed, Mar 28, 2018 at 05:29:42PM +0300, Gleb wrote:
> Ensure -Werror -Wall set for the whole src/.
> Fix warnings which have been find with prev flags.
> 
> Fixes #3238

Please, remove the dot at the end of the commit title.

> ---
> -Issue from https://github.com/tarantool/tarantool/issues/3238.
> -Source from https://github.com/tarantool/tarantool/tree/gh-3238-check-warnings.
>  src/box/sql/alter.c        |  2 +-
>  src/box/sql/pragma.c       |  3 +--
>  src/box/vy_read_iterator.c |  2 +-
>  src/box/xrow.c             | 10 ++++++----
>  src/httpc.c                |  2 +-
>  src/lua/init.c             |  2 +-
>  src/say.c                  |  6 ++++--
>  7 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index a52ba8d..098d183 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -150,7 +150,7 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
>  	Column *pCol;		/* The new column */
>  	Expr *pDflt;		/* Default value for the new column */
>  	sqlite3 *db;		/* The database connection; */
> -	Vdbe *v = pParse->pVdbe;	/* The prepared statement under construction */
> +	Vdbe *v __attribute__((unused)) = pParse->pVdbe;	/* The prepared statement under construction */
>  	struct session *user_session = current_session();
>  

The line is longer that 80 symbols.

Please, use portable macro MAYBE_UNUSED from src/trivia/util.h (or cast
to void as an separate statement).

>  	db = pParse->db;
> diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
> index 23b4c73..4ecc18d 100644
> --- a/src/box/sql/pragma.c
> +++ b/src/box/sql/pragma.c
> @@ -257,7 +257,6 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
>  	char *zLeft = 0;	/* Nul-terminated UTF-8 string <id> */
>  	char *zRight = 0;	/* Nul-terminated UTF-8 string <value>, or NULL */
>  	char *zTable = 0;	/* Nul-terminated UTF-8 string <value2> or NULL */
> -	int rc;			/* return value form SQLITE_FCNTL_PRAGMA */
>  	sqlite3 *db = pParse->db;	/* The database connection */
>  	Vdbe *v = sqlite3GetVdbe(pParse);	/* Prepared statement */
>  	const PragmaName *pPragma;	/* The pragma */
> @@ -521,7 +520,7 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
>  		box_tuple_t *tuple;
>  		box_iterator_t* iter;
>  		iter = box_index_iterator(space_id, 0,ITER_ALL, key_buf, key_end);
> -		rc = box_iterator_next(iter, &tuple);
> +		int rc __attribute__((unused)) = box_iterator_next(iter, &tuple); /* return value form SQLITE_FCNTL_PRAGMA */

Long line.

The comment seems to be unrelated, it is better to remove it.

>  		assert(rc==0);
>  		for (i = 0; tuple!=NULL; i++, box_iterator_next(iter, &tuple)){
>  			/* 1 is name field number */
> diff --git a/src/box/vy_read_iterator.c b/src/box/vy_read_iterator.c
> index a265f58..93681a8 100644
> --- a/src/box/vy_read_iterator.c
> +++ b/src/box/vy_read_iterator.c
> @@ -552,7 +552,7 @@ vy_read_iterator_next_lsn(struct vy_read_iterator *itr, struct tuple **ret)
>  {
>  	uint32_t i;
>  	bool unused;
> -	struct vy_read_src *src;
> +	struct vy_read_src *src = NULL;
>  
>  	assert(itr->curr_stmt != NULL);
>  	assert(itr->curr_src < itr->skipped_src);
> diff --git a/src/box/xrow.c b/src/box/xrow.c
> index 3ef3d82..b272166 100644
> --- a/src/box/xrow.c
> +++ b/src/box/xrow.c
> @@ -300,7 +300,7 @@ iproto_reply_vclock(struct obuf *out, uint64_t sync, uint32_t schema_version,
>  	iproto_header_encode(buf, IPROTO_OK, sync, schema_version,
>  			     size - IPROTO_HEADER_LEN);
>  
> -	char *ptr = obuf_alloc(out, size);
> +	char *ptr __attribute__((unused)) = obuf_alloc(out, size);
>  	assert(ptr == buf);
>  	return 0;
>  }
> @@ -339,9 +339,11 @@ iproto_write_error(int fd, const struct error *e, uint32_t schema_version,
>  			     schema_version, sizeof(body) + msg_len);
>  
>  	body.v_data_len = mp_bswap_u32(msg_len);
> -	(void) write(fd, header, sizeof(header));
> -	(void) write(fd, &body, sizeof(body));
> -	(void) write(fd, e->errmsg, msg_len);
> +	ssize_t r;
> +	r = write(fd, header, sizeof(header));
> +	r = write(fd, &body, sizeof(body));
> +	r = write(fd, e->errmsg, msg_len);
> +	(void) r;

write has __wur flag for which cast-to-void broken at times. The story
is in [1], [2] and [3]. So this is workaround for gcc bug, okay.

I afraid that 1st and 2nd *values* of `r` are ignored and gcc can give
warning about it in the future. But I'm okay if it works good from gcc
4.5 to gcc 7.2.

[1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425
[2]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25509
[3]: https://sourceware.org/bugzilla/show_bug.cgi?id=11959

>  }
>  
>  int
> diff --git a/src/httpc.c b/src/httpc.c
> index 633e688..d6c1b00 100644
> --- a/src/httpc.c
> +++ b/src/httpc.c
> @@ -249,7 +249,7 @@ httpc_set_low_speed_limit(struct httpc_request *req, long low_speed_limit)
>  void
>  httpc_set_verbose(struct httpc_request *req, bool curl_verbose)
>  {
> -	curl_easy_setopt(req->curl_request.easy, CURLOPT_VERBOSE, curl_verbose);
> +	curl_easy_setopt(req->curl_request.easy, CURLOPT_VERBOSE, (long)curl_verbose);
>  }
>  

Too long line (tab is 8 symbols long), please, break it into parts.
Separate cast operator from an expression by a space.

>  void
> diff --git a/src/lua/init.c b/src/lua/init.c
> index 76e978c..84f81e6 100644
> --- a/src/lua/init.c
> +++ b/src/lua/init.c
> @@ -279,7 +279,7 @@ tarantool_lua_setpaths(struct lua_State *L)
>  {
>  	const char *home = getenv("HOME");
>  	char cwd[PATH_MAX] = {'\0'};
> -	getcwd(cwd, sizeof(cwd));
> +	char *buf __attribute__((unused)) = getcwd(cwd, sizeof(cwd));
>  	lua_getglobal(L, "package");
>  	int top = lua_gettop(L);
>  
> diff --git a/src/say.c b/src/say.c
> index b92514d..0801f04 100644
> --- a/src/say.c
> +++ b/src/say.c
> @@ -1040,8 +1040,10 @@ log_vsay(struct log *log, int level, const char *filename, int line,
>  		break;
>  	case SAY_LOGGER_SYSLOG:
>  		write_to_syslog(log, total);
> -		if (level == S_FATAL && log->fd != STDERR_FILENO)
> -			(void) write(STDERR_FILENO, buf, total);
> +		if (level == S_FATAL && log->fd != STDERR_FILENO) {
> +			ssize_t r = write(STDERR_FILENO, buf, total);
> +			(void) r;                       /* silence gcc warning */

Why do you fill 23 spaces before the comment? Maybe just 1 is better?

> +		}
>  		break;
>  	case SAY_LOGGER_BOOT
>  	{
> -- 
> 2.7.4
>