[tarantool-patches] Re: Re[2]: Fwd: Re[2]: [PATCH] lua: add string.fromhex method

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

Hi! Thanks for the fixes! LGTM.

On 15/08/2018 17:01, Nikita Tatunov wrote:
> Hello, thank you for the review!
> See diff for src/lua/string.lua in the end of the letter.
> 
> 
>> Понедельник, 13 августа 2018, 18:05 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:
>>
>> Hi!
>>
>> One inline note is below.
>>
>> WBR, Alexander Turenko.
>>
>> On Aug 13, 2018 5:46 PM, Vladislav Shpilevoy < v.shpilevoy@tarantool.org > wrote:
>>> Hi! Thanks for the patch!
>>>
>>> See 3 comments below.
>>>
>>>> commit c208a8173b2f7632c3432d03807490becfe7276b
>>>> Author: N.Tatunov < n.tatunov@tarantool.org >
>>>> Date:   Wed Aug 8 14:17:02 2018 +0300
>>>>
>>>>       lua: add string.fromhex method
>>>>       
>>>>       Add string.fromhex method. Add test for string.fromhex().
>>>>       
>>>>       Closes #2562
>>>>
>>>> diff --git a/src/lua/string.lua b/src/lua/string.lua
>>>> index 5ff64c9f6..f6accb42c 100644
>>>> --- a/src/lua/string.lua
>>>> +++ b/src/lua/string.lua
>>>> @@ -292,6 +292,54 @@ local function string_hex(inp)
>>>>        return ffi.string(res, len)
>>>>    end
>>>>    
>>>> +local hexadecimal_chars = {
>>>> +    '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A',
>>>> +    'a', 'B', 'b', 'C', 'c', 'D', 'd', 'E', 'e', 'F', 'f'}
>>>> +
>>>> +local hexadecimal_values = {
>>>> +    0,  1,  2,  3,  4,  5,  6,  7,  8,  9,  10,
>>>> +    10, 11, 11, 12, 12, 13, 13, 14, 14, 15, 15}
>>>> +
>>>> +local hexadecimals_mapping = {}
>>>> +
>>>> +local function chars_to_hex(chars, values, mapping)
>>>> +    for i, char in ipairs(chars) do
>>>> +        mapping[string.byte(char)] = values[i]
>>>> +    end
>>>> +end
>>>
>>> 1. Why do you need this function to be called only once 1 line
>>> below? Can you just inline it?
>>>
> 
> 1. Fixed.
> 
>>>> +
>>>> +chars_to_hex(hexadecimal_chars, hexadecimal_values, hexadecimals_mapping)
>>>> +
>>>> +--- Match a hexadecimal representation of a string to its
>>>> +-- string representation.
>>>> +-- @function fromhex
>>>> +-- @string   inp     the string of hexadecimals
>>>> +-- @returns          formatted string
>>>
>>> 2. I know that such comments style you got from other functions
>>> of the file, but each line above contradicts with our official
>>> code style, as I understand. Just for record.
>>>
> 
> 2. It's a codestyle from lua style guideline. As we discussed verbally
> I fixed it to the appropriate one. Also created an issue to fix the guideline (#596).
> 
>>>> +local function string_fromhex(inp)
>>>> +    if type(inp) ~= 'string' then
>>>> +        error(err_string_arg:format(1, 'string.fromhex', 'string',
>>>> +                                    type(inp)), 2)
>>>> +    end
>>>> +    if inp:len() % 2 ~= 0 then
>>>> +        error(err_string_arg:format(1, 'string.fromhex',
>>>> +                                    'even amount of chars',
>>>> +                                    'odd amount'), 2)
>>>> +    end
>>>> +    local len = inp:len() / 2
>>>> +    local casted_inp = ffi.cast('const char *', inp)
>>>> +    local res = ffi.new('char[?]', len)
>>>
>>> 3. Why '?'? You know that res will be exactly len bytes.
>>
>> Concatenation in Lua is slower than providing a type and a length to ffi.new function separately. It is according to benchmarking results.
>>
> 
> 3. As Alexander told it is faster.
> 
>>>
>>>> +    for i = 0, len - 1 do
>>>> +        local first = hexadecimals_mapping[casted_inp[i * 2]]
>>>> +        local second = hexadecimals_mapping[casted_inp[i * 2 + 1]]
>>>> +        if first == nil or second == nil then
>>>> +            error(err_string_arg:format(1, 'string.fromhex', 'hex string',
>>>> +                                        'non hex chars'), 2)
>>>> +        end
>>>> +        res[i] = first * 16 + second
>>>> +    end
>>>> +    return ffi.string(res, len)
>>>> +end
>>>> +
>>>>    local function string_strip(inp)
>>>>        if type(inp) ~= 'string' then
>>>>            error(err_string_arg:format(1, "string.strip", 'string', type(inp)), 2)
>>>
>>
> 
> diff --git a/src/lua/string.lua b/src/lua/string.lua
> index 5ff64c9f6..cbce26b35 100644
> --- a/src/lua/string.lua
> +++ b/src/lua/string.lua
> @@ -292,6 +292,53 @@ local function string_hex(inp)
>       return ffi.string(res, len)
>   end
>   
> +local hexadecimal_chars = {
> +    '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A',
> +    'a', 'B', 'b', 'C', 'c', 'D', 'd', 'E', 'e', 'F', 'f'}
> +
> +local hexadecimal_values = {
> +    0,  1,  2,  3,  4,  5,  6,  7,  8,  9,  10,
> +    10, 11, 11, 12, 12, 13, 13, 14, 14, 15, 15}
> +
> +local hexadecimals_mapping = {}
> +
> +for i, char in ipairs(hexadecimal_chars) do
> +    hexadecimals_mapping[string.byte(char)] = hexadecimal_values[i]
> +end
> +
> +
> +--
> +-- Match a hexadecimal representation of a string to its
> +-- string representation.
> +-- @param inp the string of hexadecimals
> +--
> +-- @retval formatted string
> +--
> +local function string_fromhex(inp)
> +    if type(inp) ~= 'string' then
> +        error(err_string_arg:format(1, 'string.fromhex', 'string',
> +                                    type(inp)), 2)
> +    end
> +    if inp:len() % 2 ~= 0 then
> +        error(err_string_arg:format(1, 'string.fromhex',
> +                                    'even amount of chars',
> +                                    'odd amount'), 2)
> +    end
> +    local len = inp:len() / 2
> +    local casted_inp = ffi.cast('const char *', inp)
> +    local res = ffi.new('char[?]', len)
> +    for i = 0, len - 1 do
> +        local first = hexadecimals_mapping[casted_inp[i * 2]]
> +        local second = hexadecimals_mapping[casted_inp[i * 2 + 1]]
> +        if first == nil or second == nil then
> +            error(err_string_arg:format(1, 'string.fromhex', 'hex string',
> +                                        'non hex chars'), 2)
> +        end
> +        res[i] = first * 16 + second
> +    end
> +    return ffi.string(res, len)
> +end
> +
>   local function string_strip(inp)
>       if type(inp) ~= 'string' then
>           error(err_string_arg:format(1, "string.strip", 'string', type(inp)), 2)
> @@ -323,6 +370,7 @@ string.center     = string_center
>   string.startswith = string_startswith
>   string.endswith   = string_endswith
>   string.hex        = string_hex
> +string.fromhex    = string_fromhex
>   string.strip      = string_strip
>   string.lstrip      = string_lstrip
>   string.rstrip      = string_rstrip
> 
>