<div dir='auto'><div>Hi!</div><div dir="auto"><br></div><div dir="auto">One inline note is below.</div><div dir="auto"><br></div><div dir="auto">WBR, Alexander Turenko.<br><div class="gmail_extra" dir="auto"><br><div class="gmail_quote">On Aug 13, 2018 5:46 PM, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:<br type="attribution"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><p dir="ltr">Hi! Thanks for the patch!
<br>

<br>
See 3 comments below.
<br>

<br>
> commit c208a8173b2f7632c3432d03807490becfe7276b
<br>
> Author: N.Tatunov <n.tatunov@tarantool.org>
<br>
> Date:   Wed Aug 8 14:17:02 2018 +0300
<br>
> 
<br>
>     lua: add string.fromhex method
<br>
>     
<br>
>     Add string.fromhex method. Add test for string.fromhex().
<br>
>     
<br>
>     Closes #2562
<br>
> 
<br>
> diff --git a/src/lua/string.lua b/src/lua/string.lua
<br>
> index 5ff64c9f6..f6accb42c 100644
<br>
> --- a/src/lua/string.lua
<br>
> +++ b/src/lua/string.lua
<br>
> @@ -292,6 +292,54 @@ local function string_hex(inp)
<br>
>      return ffi.string(res, len)
<br>
>  end
<br>
>  
<br>
> +local hexadecimal_chars = {
<br>
> +    '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A',
<br>
> +    'a', 'B', 'b', 'C', 'c', 'D', 'd', 'E', 'e', 'F', 'f'}
<br>
> +
<br>
> +local hexadecimal_values = {
<br>
> +    0,  1,  2,  3,  4,  5,  6,  7,  8,  9,  10,
<br>
> +    10, 11, 11, 12, 12, 13, 13, 14, 14, 15, 15}
<br>
> +
<br>
> +local hexadecimals_mapping = {}
<br>
> +
<br>
> +local function chars_to_hex(chars, values, mapping)
<br>
> +    for i, char in ipairs(chars) do
<br>
> +        mapping[string.byte(char)] = values[i]
<br>
> +    end
<br>
> +end
<br>

<br>
1. Why do you need this function to be called only once 1 line
<br>
below? Can you just inline it?
<br>

<br>
> +
<br>
> +chars_to_hex(hexadecimal_chars, hexadecimal_values, hexadecimals_mapping)
<br>
> +
<br>
> +--- Match a hexadecimal representation of a string to its
<br>
> +-- string representation.
<br>
> +-- @function fromhex
<br>
> +-- @string   inp     the string of hexadecimals
<br>
> +-- @returns          formatted string
<br>

<br>
2. I know that such comments style you got from other functions
<br>
of the file, but each line above contradicts with our official
<br>
code style, as I understand. Just for record.
<br>

<br>
> +local function string_fromhex(inp)
<br>
> +    if type(inp) ~= 'string' then
<br>
> +        error(err_string_arg:format(1, 'string.fromhex', 'string',
<br>
> +                                    type(inp)), 2)
<br>
> +    end
<br>
> +    if inp:len() % 2 ~= 0 then
<br>
> +        error(err_string_arg:format(1, 'string.fromhex',
<br>
> +                                    'even amount of chars',
<br>
> +                                    'odd amount'), 2)
<br>
> +    end
<br>
> +    local len = inp:len() / 2
<br>
> +    local casted_inp = ffi.cast('const char *', inp)
<br>
> +    local res = ffi.new('char[?]', len)
<br>

<br>
3. Why '?'? You know that res will be exactly len bytes.
<br>

</p></blockquote></div></div></div><div dir="auto"><br></div><div dir="auto">Concatenation in Lua is slower than providing a type and a length to ffi.new function separately. It is according to benchmarking results.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><p dir="ltr"><br>
> +    for i = 0, len - 1 do
<br>
> +        local first = hexadecimals_mapping[casted_inp[i * 2]]
<br>
> +        local second = hexadecimals_mapping[casted_inp[i * 2 + 1]]
<br>
> +        if first == nil or second == nil then
<br>
> +            error(err_string_arg:format(1, 'string.fromhex', 'hex string',
<br>
> +                                        'non hex chars'), 2)
<br>
> +        end
<br>
> +        res[i] = first * 16 + second
<br>
> +    end
<br>
> +    return ffi.string(res, len)
<br>
> +end
<br>
> +
<br>
>  local function string_strip(inp)
<br>
>      if type(inp) ~= 'string' then
<br>
>          error(err_string_arg:format(1, "string.strip", 'string', type(inp)), 2)
<br>

<br>
</p>
</blockquote></div><br></div></div></div>