[tarantool-patches] Re: [tarantool-patches] Re: Re[2]: Fwd: Re[2]: [PATCH] lua: add string.fromhex method
Nikita Tatunov
n.tatunov at tarantool.org
Wed Aug 15 17:01:21 MSK 2018
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 at tarantool.org>:
>
>Hi!
>
>One inline note is below.
>
>WBR, Alexander Turenko.
>
>On Aug 13, 2018 5:46 PM, Vladislav Shpilevoy < v.shpilevoy at tarantool.org > wrote:
>>Hi! Thanks for the patch!
>>
>>See 3 comments below.
>>
>>> commit c208a8173b2f7632c3432d03807490becfe7276b
>>> Author: N.Tatunov < n.tatunov at 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.
More information about the Tarantool-patches
mailing list