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

["Nikita Tatunov" <n.tatunov@tarantool.org>]

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


-- 
WBR, Nikita Tatunov.