From: Alexander Turenko <alexander.turenko@tarantool.org> To: "Michał Durak" <gdrbyko1@protonmail.com> Cc: "tarantool-patches@freelists.org" <tarantool-patches@freelists.org> Subject: [tarantool-patches] Re: [PATCH v3] lua: add 'chars' param to string.strip functions Date: Mon, 4 Feb 2019 07:47:27 +0300 [thread overview] Message-ID: <20190204044727.nwtouqzg65mqngiv@tkn_work_nb> (raw) In-Reply-To: <cIcu3spd2UbJ4XiYJPOB0ZNZde1-uRC_EhWsmLEVHP3qZti2r40pA8u9-l2m_Wv6LZBuiqgEzKdfsCJ1vj08SgspRpSVTyXZTEgo2X3kzXM=@protonmail.com> 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
next prev parent reply other threads:[~2019-02-04 4:47 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-01-22 18:51 [tarantool-patches] " Michał Durak 2019-02-04 4:47 ` Alexander Turenko [this message] 2019-02-11 19:20 ` [tarantool-patches] " Michał Durak 2019-02-18 20:42 ` Alexander Turenko 2019-02-21 13:01 ` Vladimir Davydov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190204044727.nwtouqzg65mqngiv@tkn_work_nb \ --to=alexander.turenko@tarantool.org \ --cc=gdrbyko1@protonmail.com \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v3] lua: add '\''chars'\'' param to string.strip functions' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox