From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 5B7622DABC for ; Fri, 26 Apr 2019 06:18:42 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ukBqbl1DMYlS for ; Fri, 26 Apr 2019 06:18:42 -0400 (EDT) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 79D882DAB3 for ; Fri, 26 Apr 2019 06:18:41 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs References: <64db98703d236c2e6775601a48fb19c3bc8ba955.1556241512.git.alexander.turenko@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 26 Apr 2019 13:18:28 +0300 MIME-Version: 1.0 In-Reply-To: <64db98703d236c2e6775601a48fb19c3bc8ba955.1556241512.git.alexander.turenko@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Alexander Turenko Cc: tarantool-patches@freelists.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 >