[tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

Firstly, after your patch I can't build Tarantool with gcc-7
on Mac with this new -DENABLE_WERROR=ON option. I get these
errors:

error: unknown warning option '-Wno-format-truncation' [-Werror,-Wunknown-warning-option]
errorScanning dependencies of target rope_stress.test
error: unknown warning option '-Wno-format-truncation' [-Werror,-Wunknown-warning-option]
error: unknown warning option '-Wno-cast-function-type'; did you mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]
error: unknown warning option '-Wno-format-truncation' [-Werror,-Wunknown-warning-option]
error: unknown warning option '-Wno-cast-function-type'; did you mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]

After that I found that master branch also can't be built, but
for another reason:
https://github.com/tarantool/tarantool/issues/4184

I think, that your commit should not exacerbate it, at least.

See 9 comments below, and my fixes on the branch in a separate commit.

> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 2dfc4fc9d..14177aaab 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -689,12 +689,12 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
>  	if (map_size == 2) {
>  		key = mp_decode_uint(data);
>  		assert(key == SQL_INFO_AUTOINCREMENT_IDS);
> -		(void)key;
> +		(void) key;

1. Unnecessary diff. Please, drop.

>  		uint64_t count = mp_decode_array(data);
>  		assert(count > 0);
>  		lua_createtable(L, 0, count);
>  		for (uint32_t j = 0; j < count; ++j) {
> -			int64_t id;
> +			int64_t id = INT64_MIN;

2. Is there any concrete reason why the value is so
non-trivial? Why not just simple zero?

>  			mp_read_int64(data, &id);
>  			luaL_pushint64(L, id);
>  			lua_rawseti(L, -2, j + 1);
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index f3790ba01..f96709a75 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -761,12 +761,13 @@ sql_utf8_pattern_compare(const char *pattern,
>  				if (c != MATCH_ALL_WILDCARD &&
>  				    c != MATCH_ONE_WILDCARD)
>  					break;
> -				if (c == MATCH_ONE_WILDCARD &&
> -				    (c2 = Utf8Read(string, string_end)) ==
> -				    SQL_END_OF_STRING)
> -					return NO_WILDCARD_MATCH;
> -				if (c2 == SQL_INVALID_UTF8_SYMBOL)
> -					return NO_MATCH;
> +				if (c == MATCH_ONE_WILDCARD) {

3. Now you check for 'c == MATCH_ONE_WILDCARD' two times: here and
two lines above. Please, consider my fixes:

=====================================================================
 			       SQL_END_OF_STRING) {
 				if (c == SQL_INVALID_UTF8_SYMBOL)
 					return INVALID_PATTERN;
-				if (c != MATCH_ALL_WILDCARD &&
-				    c != MATCH_ONE_WILDCARD)
-					break;
 				if (c == MATCH_ONE_WILDCARD) {
 					c2 = Utf8Read(string, string_end);
 					if (c2 == SQL_INVALID_UTF8_SYMBOL)
 						return NO_MATCH;
 					if (c2 == SQL_END_OF_STRING)
 						return NO_WILDCARD_MATCH;
+				} else if (c != MATCH_ALL_WILDCARD) {
+					break;
 				}
 			}
 			/*
=====================================================================

> +					c2 = Utf8Read(string, string_end);
> +					if (c2 == SQL_INVALID_UTF8_SYMBOL)
> +						return NO_MATCH;
> +					if (c2 == SQL_END_OF_STRING)
> +						return NO_WILDCARD_MATCH;
> +				}
>  			}
>  			/*
>  			 * "%" at the end of the pattern matches.
> diff --git a/src/box/vy_cache.c b/src/box/vy_cache.c
> index be11b986f..7007d0e35 100644
> --- a/src/box/vy_cache.c
> +++ b/src/box/vy_cache.c
> @@ -661,7 +661,7 @@ vy_cache_iterator_seek(struct vy_cache_iterator *itr, struct vy_entry last)
>  				ITER_GT : ITER_LT;
>  	}
>  
> -	bool exact;
> +	bool exact = false;
>  	if (!vy_stmt_is_empty_key(key.stmt)) {
>  		itr->curr_pos = iterator_type == ITER_EQ ||
>  				iterator_type == ITER_GE ||
> diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c
> index 9513d5b80..a4fae26e2 100644
> --- a/src/box/vy_mem.c
> +++ b/src/box/vy_mem.c
> @@ -392,7 +392,7 @@ vy_mem_iterator_seek(struct vy_mem_iterator *itr, struct vy_entry last)
>  				ITER_GT : ITER_LT;
>  	}
>  
> -	bool exact;
> +	bool exact = false;

4. Sorry, can't review Vinyl part, because I do not understand this code.
But anyway your solution is better than an uninitialized variable. By the way,
can it be the problem Kirill investigates right now, when some tuples are
not returned from 'select'?

>  	struct vy_mem_tree_key tree_key;
>  	tree_key.entry = key;
>  	/* (lsn == INT64_MAX - 1) means that lsn is ignored in comparison */
> diff --git a/src/box/xrow.c b/src/box/xrow.c
> index aaed84e38..ccdd9def5 100644
> --- a/src/box/xrow.c
> +++ b/src/box/xrow.c
> @@ -509,9 +509,14 @@ 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);
> +
> +	int rc1, rc2, rc3;
> +	rc1 = write(fd, header, sizeof(header));
> +	rc2 = write(fd, &body, sizeof(body));
> +	rc3 = write(fd, e->errmsg, msg_len);
> +	(void) rc1;
> +	(void) rc2;
> +	(void) rc3;

5. Firstly, you could use one 'rc' for all of them. It would be shorter.
Secondly, why not just remove these (void) casts? We ignore returned value
in many places and it is ok, if it isn't really important.

In this concrete place an error has already happened, and it makes
no sense to create one another - this is why the values are ignored.

=====================================================================
 	body.v_data_len = mp_bswap_u32(msg_len);
-
-	int rc1, rc2, rc3;
-	rc1 = write(fd, header, sizeof(header));
-	rc2 = write(fd, &body, sizeof(body));
-	rc3 = write(fd, e->errmsg, msg_len);
-	(void) rc1;
-	(void) rc2;
-	(void) rc3;
+	write(fd, header, sizeof(header));
+	write(fd, &body, sizeof(body));
+	write(fd, e->errmsg, msg_len);
=====================================================================

>  }
>  
>  int
> @@ -624,12 +629,15 @@ xrow_decode_dml(struct xrow_header *row, struct request *request,
>  	request->header = row;
>  	request->type = row->type;
>  
> +	const char *start = NULL;
> +	const char *end = NULL;
> +
>  	if (row->bodycnt == 0)
>  		goto done;
>  
>  	assert(row->bodycnt == 1);
> -	const char *data = (const char *) row->body[0].iov_base;
> -	const char *end = data + row->body[0].iov_len;
> +	const char *data = start = (const char *) row->body[0].iov_base;
> +	end = data + row->body[0].iov_len;
>  	assert((end - data) > 0);
>  
>  	if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
> @@ -701,8 +709,8 @@ error:
>  done:
>  	if (key_map) {
>  		enum iproto_key key = (enum iproto_key) bit_ctz_u64(key_map);
> -		xrow_on_decode_err(row->body[0].iov_base, end,
> -				   ER_MISSING_REQUEST_FIELD, iproto_key_name(key));
> +		xrow_on_decode_err(start, end, ER_MISSING_REQUEST_FIELD,
> +				   iproto_key_name(key));

6. Why did you add 'start' variable? This code was fully okay, as I
understand. Compiler could not raise an error on 'row->body[0].iov_base'
usage here. Please, drop this hunk. A patch should not contain unrelated
changes making review harder and git history dirtier.

>  		return -1;
>  	}
>  	return 0;
> @@ -1071,12 +1079,16 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
>  	ballot->is_ro = false;
>  	vclock_create(&ballot->vclock);
>  
> +	const char *start = NULL;
> +	const char *end = NULL;
> +
>  	if (row->bodycnt == 0)
>  		goto err;
> +

7. Excessive empty line. Please, drop.

>  	assert(row->bodycnt == 1);
>  
> -	const char *data = (const char *) row->body[0].iov_base;
> -	const char *end = data + row->body[0].iov_len;
> +	const char *data = start = (const char *) row->body[0].iov_base;
> +	end = data + row->body[0].iov_len;
>  	const char *tmp = data;
>  	if (mp_check(&tmp, end) != 0 || mp_typeof(*data) != MP_MAP)
>  		goto err;
> @@ -1124,8 +1136,7 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
>  	}
>  	return 0;
>  err:
> -	xrow_on_decode_err(row->body[0].iov_base, end, ER_INVALID_MSGPACK,
> -			   "packet body");
> +	xrow_on_decode_err(start, end, ER_INVALID_MSGPACK, "packet body");

8. The same as 6.

>  	return -1;
>  }
>  
> diff --git a/src/lua/init.c b/src/lua/init.c
> index d18c8af94..f66a3eb46 100644
> --- a/src/lua/init.c
> +++ b/src/lua/init.c
> @@ -319,7 +319,9 @@ tarantool_lua_setpaths(struct lua_State *L)
>  {
>  	const char *home = getenv("HOME");
>  	char cwd[PATH_MAX] = {'\0'};
> -	getcwd(cwd, sizeof(cwd));
> +	char *rc = getcwd(cwd, sizeof(cwd));
> +	if (rc == NULL)
> +		panic("can't get cwd; errno: %d", errno);

9. 'cwd' variable is not used at all and can be removed. It was
introduced here 9e7c421758af156d19a66cebf713e24b42d8654b but
was forgotten to remove sometime.

=====================================================================
 	const char *home = getenv("HOME");
-	char cwd[PATH_MAX] = {'\0'};
-	char *rc = getcwd(cwd, sizeof(cwd));
-	if (rc == NULL)
-		panic("can't get cwd; errno: %d", errno);
 	lua_getglobal(L, "package");
 	int top = lua_gettop(L);
=====================================================================

>  	lua_getglobal(L, "package");
>  	int top = lua_gettop(L);
>  
> -- 
> 2.21.0
>