Re: [PATCH] Optimization of getarrvalue()

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Tue, 08 Nov 2016 12:11:39 -0800
Sebastian Gniazdowski <psprint@xxxxxxxxxxxx> wrote:
> The function contains arrdup() that includes elements beyond end index.
> I've replaced it with arrdup_max() that has limit parameter – will
> duplicate at most that limit of elements.

I'm not 100% sure that it's safe to write to the source string in all
cases (in particular, if the array is special).  I've replaced this with
the following.  It shouldn't make any significant difference.

pws

diff --git a/Src/params.c b/Src/params.c
index 3f01792..772345b 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2294,14 +2294,24 @@ getarrvalue(Value v)
 	v->start += arrlen(s);
     if (v->end < 0)
 	v->end += arrlen(s) + 1;
-    if (arrlen_lt(s, v->start) || v->start < 0)
+
+    /* Null if 1) array too short, 2) index still negative */
+    if (arrlen_lt(s, v->start) || v->start < 0) {
 	s = arrdup(nular);
-    else
-	s = arrdup(s + v->start);
-    if (v->end <= v->start)
-	s[0] = NULL;
-    else if (arrlen_ge(s, v->end - v->start))
-	s[v->end - v->start] = NULL;
+    } else if (v->end <= v->start) {
+        s = arrdup_max(s, 1);
+        s[0] = NULL;
+    } else {
+        /* Copy to a point before the end of the source array:
+         * arrdup_max will copy at most v->end - v->start elements,
+         * starting from v->start element. Original code said:
+	 *  s[v->end - v->start] = NULL
+         * which means that there are exactly the same number of
+         * elements as the value of the above *0-based* index.
+         */
+	s = arrdup_max(s + v->start, v->end - v->start);
+    }
+
     return s;
 }
 
diff --git a/Src/utils.c b/Src/utils.c
index 93e757c..3d535b8 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -4229,6 +4229,31 @@ arrdup(char **s)
     return y;
 }
 
+/* Duplicate at most max elements of the array s with heap memory */
+
+/**/
+mod_export char **
+arrdup_max(char **s, unsigned max)
+{
+    char **x, **y, **send;
+    int len;
+
+    len = arrlen(s);
+
+    /* Limit has sense only if not equal to len */
+    if (max > len)
+        max = len;
+
+    y = x = (char **) zhalloc(sizeof(char *) * (max + 1));
+
+    send = s + max;
+    while (s < send)
+	*x++ = dupstring(*s++);
+    *x = NULL;
+
+    return y;
+}
+
 /**/
 mod_export char **
 zarrdup(char **s)