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

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

[Alexander Turenko]

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
> 

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

[Alexander Turenko]

Hi Gleb!

Please, remove set(CMAKE_C(XX)_FLAGS) from src/box/sql/CMakeLists.txt,
because it becomes redundant.

Travis-CI fails on your commit on say unit test. It seems it was fixed
in recent 2.0 branch, please rebase on 2.0 and make sure CI is passed.

The patch generally looks good for me except notes above and below.

Nikita, can you look into the patch?

On Wed, Apr 04, 2018 at 11:59:44AM +0300, Gleb wrote:

> Ensure -Werror -Wall set for the whole src/.
> Fix warnings which have been find with -Werror and -Wall.
> Add new building target RelWithDebInfoWError.
> Change building target on RelWithDebInfoWError in Ci.
> Fixes #3238

Separate 'Fixes #xxxx' with newline for readability.

Ci -> CI.

> index 05d33ab..6ec5f93 100644
> --- a/cmake/compiler.cmake
> +++ b/cmake/compiler.cmake
> @@ -251,12 +251,19 @@ macro(enable_tnt_compile_flags)
>      add_definitions("-D__STDC_LIMIT_MACROS=1")
>      add_definitions("-D__STDC_CONSTANT_MACROS=1")
>  
> -    # Only add -Werror if it's a debug build, done by developers.
> +    # Only add -Werror if it's a Debug or
> +    # RelWithDebInfoWError build, done by developers.
>      # Release builds should not cause extra trouble.
>      if ((${CMAKE_BUILD_TYPE} STREQUAL "Debug")
>          AND HAVE_STD_C11 AND HAVE_STD_CXX11)
>          add_compile_flags("C;CXX" "-Werror")
>      endif()
> +
> +    if ((${CMAKE_BUILD_TYPE} STREQUAL "RelWithDebInfoWError")
> +        AND HAVE_STD_C11 AND HAVE_STD_CXX11)
> +        add_compile_flags("C;CXX" "-Werror")
> +        message("Trouble Release")
> +    endif()
>  endmacro(enable_tnt_compile_flags)

Please, remove the message.

> @@ -1040,13 +1040,15 @@ 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 */
> +		}
>  		break;

Replace 23 spaces before comment with one.

WBR, Alexander Turenko.