patch 9.1.1508: string manipulation can be improved in cmdexpand.c

Problem:  String manipulation can be improved in cmdexpand.c
Solution: Refactor cmdexpand.c to remove calls to
          STRLEN()/STRMOVE()/STRCAT() (John Marriott)

This commit does the following:

In function nextwild():
  - slightly refactor the for loop to remove an array access
  - call STRLEN() and store it's result for reuse
  - move some variables closer to where they are used, renaming some on
    the way

In function ExpandOne():
  - move some calculations outside of the for loops
  - factor out calls to STRCAT() (which has an inherent STRLEN() call) in
    the for loop
  - move some variables closer to where they are used

In function expand_files_and_dirs():
  - factor out calls to STRMOVE() (which has an inherent STRLEN() call)

In function get_filetypecmd_arg():
  - move declarations of the string arrays into the blocks where they are
    used

In function get_breakadd_arg():
  - move declaration of the string array into the block where it is
    used

In function globpath():
  - factor out calls to STRLEN() and STRCAT()
  - move some variables closer to where they are used

And finally some minor cosmetic style changes

closes: #17639

Signed-off-by: John Marriott <basilisk@internode.on.net>
Signed-off-by: Christian Brabandt <cb@256bit.org>

src/cmdexpand.c

@@ -228,11 +228,8 @@ nextwild(
     int		escape)		// if TRUE, escape the returned matches
 {
     cmdline_info_T	*ccline = get_cmdline_info();
-    int		i, j;
+    int		i;
-    char_u	*p1;
+    char_u	*p;
-    char_u	*p2;
-    int		difflen;
-    int		v;
 
     if (xp->xp_numfiles == -1)
     {
@@ -278,20 +275,22 @@ nextwild(
 	    || type == WILD_PAGEUP || type == WILD_PAGEDOWN)
     {
 	// Get next/previous match for a previous expanded pattern.
-	p2 = ExpandOne(xp, NULL, NULL, 0, type);
+	p = ExpandOne(xp, NULL, NULL, 0, type);
     }
     else
     {
+	char_u	*tmp;
+
 	if (cmdline_fuzzy_completion_supported(xp)
 		|| xp->xp_context == EXPAND_PATTERN_IN_BUF)
 	    // Don't modify the search string
-	    p1 = vim_strnsave(xp->xp_pattern, xp->xp_pattern_len);
+	    tmp = vim_strnsave(xp->xp_pattern, xp->xp_pattern_len);
 	else
-	    p1 = addstar(xp->xp_pattern, xp->xp_pattern_len, xp->xp_context);
+	    tmp = addstar(xp->xp_pattern, xp->xp_pattern_len, xp->xp_context);
 
 	// Translate string into pattern and expand it.
-	if (p1 == NULL)
+	if (tmp == NULL)
-	    p2 = NULL;
+	    p = NULL;
 	else
 	{
 	    int use_options = options |
@@ -303,26 +302,34 @@ nextwild(
 	    if (p_wic)
 		use_options += WILD_ICASE;
 
-	    p2 = ExpandOne(xp, p1,
+	    p = ExpandOne(xp, tmp,
 			 vim_strnsave(&ccline->cmdbuff[i], xp->xp_pattern_len),
 							   use_options, type);
-	    vim_free(p1);
+	    vim_free(tmp);
 	    // longest match: make sure it is not shorter, happens with :help
-	    if (p2 != NULL && type == WILD_LONGEST)
+	    if (p != NULL && type == WILD_LONGEST)
 	    {
+		int	j;
+
 		for (j = 0; j < xp->xp_pattern_len; ++j)
-		     if (ccline->cmdbuff[i + j] == '*'
+		{
-			     || ccline->cmdbuff[i + j] == '?')
+		    char_u  c = ccline->cmdbuff[i + j];
+		    if (c == '*' || c == '?')
 			break;
-		if ((int)STRLEN(p2) < j)
+		}
-		    VIM_CLEAR(p2);
+		if ((int)STRLEN(p) < j)
+		    VIM_CLEAR(p);
 	    }
 	}
     }
 
-    if (p2 != NULL && !got_int)
+    if (p != NULL && !got_int)
     {
-	difflen = (int)STRLEN(p2) - xp->xp_pattern_len;
+	size_t	plen = STRLEN(p);
+	int	difflen;
+	int	v;
+
+	difflen = (int)plen - xp->xp_pattern_len;
 	if (ccline->cmdlen + difflen + 4 > ccline->cmdbufflen)
 	{
 	    v = realloc_cmdbuff(ccline->cmdlen + difflen + 4);
@@ -335,7 +342,7 @@ nextwild(
 	    mch_memmove(&ccline->cmdbuff[ccline->cmdpos + difflen],
 		    &ccline->cmdbuff[ccline->cmdpos],
 		    (size_t)(ccline->cmdlen - ccline->cmdpos + 1));
-	    mch_memmove(&ccline->cmdbuff[i], p2, STRLEN(p2));
+	    mch_memmove(&ccline->cmdbuff[i], p, plen);
 	    ccline->cmdlen += difflen;
 	    ccline->cmdpos += difflen;
 	}
@@ -346,16 +353,16 @@ nextwild(
 
     // When expanding a ":map" command and no matches are found, assume that
     // the key is supposed to be inserted literally
-    if (xp->xp_context == EXPAND_MAPPINGS && p2 == NULL)
+    if (xp->xp_context == EXPAND_MAPPINGS && p == NULL)
 	return FAIL;
 
-    if (xp->xp_numfiles <= 0 && p2 == NULL)
+    if (xp->xp_numfiles <= 0 && p == NULL)
 	beep_flush();
     else if (xp->xp_numfiles == 1 && !(options & WILD_KEEP_SOLE_ITEM))
 	// free expanded pattern
 	(void)ExpandOne(xp, NULL, NULL, 0, WILD_FREE);
 
-    vim_free(p2);
+    vim_free(p);
 
     return OK;
 }
@@ -1023,8 +1030,6 @@ ExpandOne(
 {
     char_u	*ss = NULL;
     int		orig_saved = FALSE;
-    int		i;
-    long_u	len;
 
     // first handle the case of using an old match
     if (mode == WILD_NEXT || mode == WILD_PREV
@@ -1074,35 +1079,41 @@ ExpandOne(
     // and the result probably won't be used.
     if (mode == WILD_ALL && xp->xp_numfiles > 0 && !got_int)
     {
-	len = 0;
+	size_t	ss_size = 0;
-	for (i = 0; i < xp->xp_numfiles; ++i)
+	char	*prefix = "";
-	{
+	char	*suffix = (options & WILD_USE_NL) ? "\n" : " ";
-	    if (i > 0)
+	int	n = xp->xp_numfiles - 1;
-	    {
+	int	i;
+
 	if (xp->xp_prefix == XP_PREFIX_NO)
-		    len += 2;	// prefix "no"
+	{
+	    prefix = "no";
+	    ss_size = STRLEN_LITERAL("no") * n;
+	}
 	else if (xp->xp_prefix == XP_PREFIX_INV)
-		    len += 3;	// prefix "inv"
+	{
+	    prefix = "inv";
+	    ss_size = STRLEN_LITERAL("inv") * n;
 	}
-	    len += (long_u)STRLEN(xp->xp_files[i]) + 1;
+
-	}
+	for (i = 0; i < xp->xp_numfiles; ++i)
-	ss = alloc(len);
+	    ss_size += STRLEN(xp->xp_files[i]) + 1;	// +1 for the suffix
+	++ss_size;					// +1 for the NUL
+
+	ss = alloc(ss_size);
 	if (ss != NULL)
 	{
-	    *ss = NUL;
+	    size_t  ss_len = 0;
+
 	    for (i = 0; i < xp->xp_numfiles; ++i)
 	    {
-		if (i > 0)
+		ss_len += vim_snprintf_safelen(
-		{
+		    (char *)ss + ss_len,
-		    if (xp->xp_prefix == XP_PREFIX_NO)
+		    ss_size - ss_len,
-			STRCAT(ss, "no");
+		    "%s%s%s",
-		    else if (xp->xp_prefix == XP_PREFIX_INV)
+		    (i > 0) ? prefix : "",
-			STRCAT(ss, "inv");
+		    (char *)xp->xp_files[i],
-		}
+		    (i < n) ? suffix : "");
-		STRCAT(ss, xp->xp_files[i]);
-
-		if (i != xp->xp_numfiles - 1)
-		    STRCAT(ss, (options & WILD_USE_NL) ? "\n" : " ");
 	    }
 	}
     }
@@ -2982,40 +2993,70 @@ expand_files_and_dirs(
 	int		options)
 {
     int		free_pat = FALSE;
-    int		i;
     int		ret = FAIL;
 
     // for ":set path=" and ":set tags=" halve backslashes for escaped
     // space
     if (xp->xp_backslash != XP_BS_NONE)
     {
+	size_t	pat_len;
+	char_u	*pat_end;
+	char_u	*p;
+
 	free_pat = TRUE;
-	pat = vim_strsave(pat);
+
+	pat_len = STRLEN(pat);
+	pat = vim_strnsave(pat, pat_len);
 	if (pat == NULL)
 	    return ret;
 
-	for (i = 0; pat[i]; ++i)
+	pat_end = pat + pat_len;
-	    if (pat[i] == '\\')
+	for (p = pat; *p != NUL; ++p)
 	{
+	    char_u  *from;
+
+	    if (*p != '\\')
+		continue;
+
 	    if (xp->xp_backslash & XP_BS_THREE
-			&& pat[i + 1] == '\\'
+		&& *(p + 1) == '\\'
-			&& pat[i + 2] == '\\'
+		&& *(p + 2) == '\\'
-			&& pat[i + 3] == ' ')
+		&& *(p + 3) == ' ')
-		    STRMOVE(pat + i, pat + i + 3);
+	    {
+		from = p + 3;
+		mch_memmove(p, from,
+		    (size_t)(pat_end - from) + 1);   // +1 for NUL
+		pat_end -= 3;
+	    }
 	    else if (xp->xp_backslash & XP_BS_ONE
-			&& pat[i + 1] == ' ')
+		&& *(p + 1) == ' ')
-		    STRMOVE(pat + i, pat + i + 1);
+	    {
-		else if ((xp->xp_backslash & XP_BS_COMMA)
+		from = p + 1;
-			&& pat[i + 1] == '\\'
+		mch_memmove(p, from,
-			&& pat[i + 2] == ',')
+		    (size_t)(pat_end - from) + 1);   // +1 for NUL
-		    STRMOVE(pat + i, pat + i + 2);
+		--pat_end;
+	    }
+	    else if (xp->xp_backslash & XP_BS_COMMA)
+	    {
+		if (*(p + 1) == '\\' && *(p + 2) == ',')
+		{
+		    from = p + 2;
+		    mch_memmove(p, from,
+			(size_t)(pat_end - from) + 1);   // +1 for NUL
+		    pat_end -= 2;
+		}
 #ifdef BACKSLASH_IN_FILENAME
-		else if ((xp->xp_backslash & XP_BS_COMMA)
+		else if (*(p + 1) == ',')
-			&& pat[i + 1] == ',')
+		{
-		    STRMOVE(pat + i, pat + i + 1);
+		    from = p + 1;
+		    mch_memmove(p, from,
+			(size_t)(pat_end - from) + 1);   // +1 for NUL
+		    --pat_end;
+		}
 #endif
 	    }
 	}
+    }
 
     if (xp->xp_context == EXPAND_FINDFUNC)
     {
@@ -3085,19 +3126,29 @@ get_behave_arg(expand_T *xp UNUSED, int idx)
     static char_u *
 get_filetypecmd_arg(expand_T *xp UNUSED, int idx)
 {
-    char *opts_all[] = {"indent", "plugin", "on", "off"};
+    if (idx < 0)
-    char *opts_plugin[] = {"plugin", "on", "off"};
+	return NULL;
-    char *opts_indent[] = {"indent", "on", "off"};
-    char *opts_onoff[] = {"on", "off"};
 
     if (filetype_expand_what == EXP_FILETYPECMD_ALL && idx < 4)
+    {
+	char	*opts_all[] = {"indent", "plugin", "on", "off"};
 	return (char_u *)opts_all[idx];
+    }
     if (filetype_expand_what == EXP_FILETYPECMD_PLUGIN && idx < 3)
+    {
+	char	*opts_plugin[] = {"plugin", "on", "off"};
 	return (char_u *)opts_plugin[idx];
+    }
     if (filetype_expand_what == EXP_FILETYPECMD_INDENT && idx < 3)
+    {
+	char	*opts_indent[] = {"indent", "on", "off"};
 	return (char_u *)opts_indent[idx];
+    }
     if (filetype_expand_what == EXP_FILETYPECMD_ONOFF && idx < 2)
+    {
+	char	*opts_onoff[] = {"on", "off"};
 	return (char_u *)opts_onoff[idx];
+    }
     return NULL;
 }
 
@@ -3110,10 +3161,10 @@ get_filetypecmd_arg(expand_T *xp UNUSED, int idx)
     static char_u *
 get_breakadd_arg(expand_T *xp UNUSED, int idx)
 {
-    char *opts[] = {"expr", "file", "func", "here"};
-
     if (idx >= 0 && idx <= 3)
     {
+	char	*opts[] = {"expr", "file", "func", "here"};
+
 	// breakadd {expr, file, func, here}
 	if (breakpt_expand_what == EXP_BREAKPT_ADD)
 	    return (char_u *)opts[idx];
@@ -4015,9 +4066,8 @@ globpath(
 {
     expand_T	xpc;
     char_u	*buf;
-    int		i;
+    size_t	buflen;
-    int		num_p;
+    size_t	filelen;
-    char_u	**p;
 
     buf = alloc(MAXPATHL);
     if (buf == NULL)
@@ -4026,34 +4076,45 @@ globpath(
     ExpandInit(&xpc);
     xpc.xp_context = dirs ? EXPAND_DIRECTORIES : EXPAND_FILES;
 
+    filelen = STRLEN(file);
+
     // Loop over all entries in {path}.
     while (*path != NUL)
     {
 	// Copy one item of the path to buf[] and concatenate the file name.
-	copy_option_part(&path, buf, MAXPATHL, ",");
+	buflen = (size_t)copy_option_part(&path, buf, MAXPATHL, ",");
-	if (STRLEN(buf) + STRLEN(file) + 2 < MAXPATHL)
+	if (buflen + filelen + 2 < MAXPATHL)
+	{
+	    int	    num_p;
+	    char_u  **p;
+
+	    if (*buf != NUL && !after_pathsep(buf, buf + buflen))
 	    {
 #if defined(MSWIN)
 		// Using the platform's path separator (\) makes vim incorrectly
 		// treat it as an escape character, use '/' instead.
-	    if (*buf != NUL && !after_pathsep(buf, buf + STRLEN(buf)))
+		STRCPY(buf + buflen, "/");
-		STRCAT(buf, "/");
+		++buflen;
 #else
-	    add_pathsep(buf);
+		STRCPY(buf + buflen, PATHSEPSTR);
+		buflen += STRLEN_LITERAL(PATHSEPSTR);
 #endif
-	    STRCAT(buf, file);
+	    }
+	    STRCPY(buf + buflen, file);
 	    if (ExpandFromContext(&xpc, buf, &p, &num_p,
 			     WILD_SILENT|expand_options) != FAIL && num_p > 0)
 	    {
 		ExpandEscape(&xpc, buf, num_p, p, WILD_SILENT|expand_options);
 
 		if (ga_grow(ga, num_p) == OK)
+		{
 		    // take over the pointers and put them in "ga"
-		    for (i = 0; i < num_p; ++i)
+		    for (int i = 0; i < num_p; ++i)
 		    {
 			((char_u **)ga->ga_data)[ga->ga_len] = p[i];
 			++ga->ga_len;
 		    }
+		}
 		vim_free(p);
 	    }
 	}

src/version.c

@@ -719,6 +719,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    1508,
 /**/
     1507,
 /**/