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 8759A26CC0 for ; Fri, 1 Jun 2018 08:08:56 -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 ScPltCQz0bcV for ; Fri, 1 Jun 2018 08:08:56 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 453D726C1A for ; Fri, 1 Jun 2018 08:08:56 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH] sql: fix decode analyze sample From: "n.pettik" In-Reply-To: <79583087-87c1-a36d-92c2-9daaf8be7772@tarantool.org> Date: Fri, 1 Jun 2018 15:08:51 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <887A6F07-204B-4ECC-AB40-84390619CA24@tarantool.org> References: <20180518134733.4490-1-avkhatskevich@tarantool.org> <85302581-2D9C-448F-B593-364C246D6175@tarantool.org> <79583087-87c1-a36d-92c2-9daaf8be7772@tarantool.org> 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: Alex Khatskevich Cc: tarantool-patches@freelists.org > On 1 Jun 2018, at 14:41, Alex Khatskevich = wrote: >=20 > Hi >> Put here link to the branch on GitHub, and link to the issue. > Branch: = https://github.com/tarantool/tarantool/tree/kh/gh-2860-skip-scan > Issue: https://github.com/tarantool/tarantool/issues/2860 >> I see that provided changes significantly refactor this function. >> Would you mind to fix code style for whole function to make it >> look like function from server? > Done. According to SOP conventions, you should attach diff of provided = changes, or send next version of patch. Otherwise, it is quite complicated to = comment your fixes=E2=80=A6 Firstly, lets move function=E2=80=99s comment to header and fix length: comments should fit into 66 chars. Then, rename function to = sql_stat4_column(); add struct prefixes to types which are really structs; don=E2=80=99t use = Hungarian notation; put comments above the code to be commented; don=E2=80=99t use = beforehand declaration for cycle counters (until it is really vital). With these changes code would look more elegant >> Lest just return 0 in case of success, and -1 otherwise. > I do not like this approach. Actually, it is not a suggestion: in server almost all functions obey = this rule (AFAIK). If function may fail due to different reasons, you can set error message = explicitly. For instance, on OOM: diag_set(OutOfMemory, =E2=80=A6); Except for these nitpickings, patch LGTM.