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 B81F526083 for ; Sun, 3 Feb 2019 23:47:32 -0500 (EST) 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 Ams3erVgYxu9 for ; Sun, 3 Feb 2019 23:47:32 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 D92722606C for ; Sun, 3 Feb 2019 23:47:31 -0500 (EST) Date: Mon, 4 Feb 2019 07:47:27 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH v3] lua: add 'chars' param to string.strip functions Message-ID: <20190204044727.nwtouqzg65mqngiv@tkn_work_nb> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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: =?utf-8?Q?Micha=C5=82?= Durak Cc: "tarantool-patches@freelists.org" Hi! Sorry for the late answer. Some really minor comments are below. Also I attached a patch at end of the email. I changed the string helper a bit and I think it looks simpler in that way. Are there any reason (performance?) why we should not apply this change? WBR, Alexander Turenko. > branch: https://github.com/gdrbyKo1/tarantool/tree/gdrbyko1/gh-2977 > issue: https://github.com/tarantool/tarantool/issues/2977 Please, squash your two commits. > +/* > + * Implements the lstrip functionality of string_strip_helper. > + * > + * @param inp The input string to perform lstrip on. > + * @param inp_len The length of the input string. > + * @param arr An array of 256 values defining > + * the set of characters to strip. > + * @return the number of characters to strip from the left. > + */ Nit: The return value description starts from a lower-case letter. The same for rstrip_helper(). > +/* > + * Same as lstrip_helper, but for rstrip. > + * > + * @param inp The input string to perform rstrip on. > + * @param inp_len The length of the input string. > + * @param arr An array of 256 values defining > + * the set of characters to strip. > + * @return the number of characters to strip from the right Nit: no period at the end. > +void > +string_strip_helper(const char *inp, size_t inp_len, const char *chars, > + size_t chars_len, bool lstrip, bool rstrip, > + size_t *newstart, size_t *newlen) > +{ > + size_t skipped_from_left = 0; > + size_t skipped_from_right = 0; > + uint8_t arr[256] = {0}; > + for (size_t i = 0; i < chars_len; ++i) { > + unsigned char c = chars[i]; > + arr[c] = 1; > + } > + > + if (lstrip) > + skipped_from_left = lstrip_helper(inp, inp_len, arr); > + /* > + * perform rstrip if the caller so wishes, but only if there are Nit: start from a capital letter. > + * still some characters not yet stripped by lstrip > + * for example, with chars "#" and input "###", lstrip and rstrip > + * helpers would both return 3, breaking the newlen calculation > + */ > + if (rstrip && skipped_from_left < inp_len) > + skipped_from_right = rstrip_helper(inp, inp_len, arr); > + > + *newstart = skipped_from_left; > + *newlen = inp_len - skipped_from_left - skipped_from_right; > +} > diff --git a/src/lua/string.h b/src/lua/string.h > new file mode 100644 > index 000000000..464c48031 > --- /dev/null > +++ b/src/lua/string.h > @@ -0,0 +1,55 @@ > +#ifndef TARANTOOL_STRING_H_INCLUDED > +#define TARANTOOL_STRING_H_INCLUDED Nit: We usually use full path to a file to form a sentry macro name: TARANTOOL_LUA_STRING_H_INCLUDED. ---- diff --git a/src/lua/string.c b/src/lua/string.c index 03c36c262..f117f2936 100644 --- a/src/lua/string.c +++ b/src/lua/string.c @@ -86,34 +86,31 @@ rstrip_helper(const char *inp, size_t inp_len, uint8_t *arr) * @param[in] chars_len The length of chars * @param[in] lstrip Whether to perform lstrip or not. * @param[in] rstrip Whether to perform rstrip or not. - * @param[out] newstart The position of the resulting substring - * in the input string. - * @param[out] newlen The length of the resulting substring. + * @param[out] newlen The length of the resulting string. + * + * @return The resulting string. */ -void +const char * string_strip_helper(const char *inp, size_t inp_len, const char *chars, - size_t chars_len, bool lstrip, bool rstrip, - size_t *newstart, size_t *newlen) + size_t chars_len, bool lstrip, bool rstrip, size_t *newlen) { - size_t skipped_from_left = 0; - size_t skipped_from_right = 0; uint8_t arr[256] = {0}; for (size_t i = 0; i < chars_len; ++i) { unsigned char c = chars[i]; arr[c] = 1; } - if (lstrip) - skipped_from_left = lstrip_helper(inp, inp_len, arr); - /* - * perform rstrip if the caller so wishes, but only if there are - * still some characters not yet stripped by lstrip - * for example, with chars "#" and input "###", lstrip and rstrip - * helpers would both return 3, breaking the newlen calculation - */ - if (rstrip && skipped_from_left < inp_len) - skipped_from_right = rstrip_helper(inp, inp_len, arr); + if (lstrip) { + size_t skipped_from_left = lstrip_helper(inp, inp_len, arr); + inp = inp + skipped_from_left; + inp_len = inp_len - skipped_from_left; + } + + if (rstrip) { + size_t skipped_from_right = rstrip_helper(inp, inp_len, arr); + inp_len = inp_len - skipped_from_right; + } - *newstart = skipped_from_left; - *newlen = inp_len - skipped_from_left - skipped_from_right; + *newlen = inp_len; + return inp; } diff --git a/src/lua/string.h b/src/lua/string.h index 464c48031..448f551f6 100644 --- a/src/lua/string.h +++ b/src/lua/string.h @@ -41,10 +41,9 @@ extern "C" { /** \cond public */ -void +const char * string_strip_helper(const char *inp, size_t inp_len, const char *chars, - size_t chars_len, bool lstrip, bool rstrip, - size_t *newstart, size_t *newlen); + size_t chars_len, bool lstrip, bool rstrip, size_t *newlen); /** \endcond public */ diff --git a/src/lua/string.lua b/src/lua/string.lua index 8216ace6a..0c47030f1 100644 --- a/src/lua/string.lua +++ b/src/lua/string.lua @@ -6,14 +6,13 @@ ffi.cdef[[ const char *needle, size_t needle_len); int memcmp(const char *mem1, const char *mem2, size_t num); int isspace(int c); - void + const char * string_strip_helper(const char *inp, size_t inp_len, const char *chars, size_t chars_len, bool lstrip, bool rstrip, - size_t *newstart, size_t *newlen); + size_t *newlen); ]] local c_char_ptr = ffi.typeof('const char *') -local strip_newstart = ffi.new("unsigned long[1]") local strip_newlen = ffi.new("unsigned long[1]") local memcmp = ffi.C.memcmp @@ -362,9 +361,9 @@ local function string_strip(inp, chars) end local casted_inp = c_char_ptr(inp) - ffi.C.string_strip_helper(inp, #inp, chars, #chars, true, true, - strip_newstart, strip_newlen) - return ffi.string(casted_inp + strip_newstart[0], strip_newlen[0]) + local newstr = ffi.C.string_strip_helper(inp, #inp, chars, #chars, true, + true, strip_newlen) + return ffi.string(newstr, strip_newlen[0]) end local function string_lstrip(inp, chars) @@ -383,9 +382,9 @@ local function string_lstrip(inp, chars) end local casted_inp = c_char_ptr(inp) - ffi.C.string_strip_helper(inp, #inp, chars, #chars, true, false, - strip_newstart, strip_newlen) - return ffi.string(casted_inp + strip_newstart[0], strip_newlen[0]) + local newstr = ffi.C.string_strip_helper(inp, #inp, chars, #chars, true, + false, strip_newlen) + return ffi.string(newstr, strip_newlen[0]) end local function string_rstrip(inp, chars) @@ -404,9 +403,9 @@ local function string_rstrip(inp, chars) end local casted_inp = c_char_ptr(inp) - ffi.C.string_strip_helper(inp, #inp, chars, #chars, false, true, - strip_newstart, strip_newlen) - return ffi.string(casted_inp + strip_newstart[0], strip_newlen[0]) + local newstr = ffi.C.string_strip_helper(inp, #inp, chars, #chars, false, + true, strip_newlen) + return ffi.string(newstr, strip_newlen[0]) end