[tarantool-patches] Re: [PATCH v3] lua: add 'chars' param to string.strip functions

[Alexander Turenko]

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