[tarantool-patches] Re: [PATCH] sql: fix decode analyze sample

[n.pettik]

> On 1 Jun 2018, at 14:41, Alex Khatskevich <avkhatskevich at tarantool.org> wrote:
> 
> 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…

Firstly, lets move function’s 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’t use Hungarian notation;
put comments above the code to be commented; don’t 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, …);

Except for these nitpickings, patch LGTM.