From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B86D329408 for ; Wed, 15 Aug 2018 10:06:23 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id uvo1fFfwDoNa for ; Wed, 15 Aug 2018 10:06:23 -0400 (EDT) Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6F9BA293F7 for ; Wed, 15 Aug 2018 10:06:23 -0400 (EDT) Subject: [tarantool-patches] Re: Re[2]: Fwd: Re[2]: [PATCH] lua: add string.fromhex method References: <990898e3-2b5d-6fcc-c2ce-4d348a9f6a2a@tarantool.org> <2ff2efa3-3006-4e54-acfc-0b395614d420@email.android.com> <1534341681.582343017@f105.i.mail.ru> From: Vladislav Shpilevoy Message-ID: <5e75806b-1e63-ee72-49c6-e937c92c29d5@tarantool.org> Date: Wed, 15 Aug 2018 17:06:20 +0300 MIME-Version: 1.0 In-Reply-To: <1534341681.582343017@f105.i.mail.ru> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Nikita Tatunov , tarantool-patches@freelists.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 : >> >> 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 > >