Re: [PATCH] Add Strrplc() To Tapsets

[Josh Stone <jistone at redhat dot com>]

On 06/15/2009 12:50 AM, Varun Chandramohan wrote:
> Hi,
> 
> This patch adds a search and replace string functionality to existing
> tapsets. I found this function to be helpful in many use cases. The
> functionality is as follows: The function takes in a parent string
> and searches for a substring as specified by the user. If substing
> not found, the parent string is returned. If substring is found, it
> is replaced by another string of the same lenght as substring and
> returned.
> 
> NOTE: The function will search and replace all the occurance of
> substrings in a parent string when matched.
> 
> Signed-off-by: Varun Chandramohan <varunc@linux.vnet.ibm.com>

Hi Varun,

This seems like a good function to have, thanks.  I find strrplc() a bit
cryptic though -- How about str_replace() or even just replace()?

> ---
>  tapset/string.stp |   36 ++++++++++++++++++++++++++++++++++++
>  1 files changed, 36 insertions(+), 0 deletions(-)
> 
> diff --git a/tapset/string.stp b/tapset/string.stp
> index 35ee9fa..b277479 100644
> --- a/tapset/string.stp
> +++ b/tapset/string.stp
> @@ -92,6 +92,42 @@ function tokenize:string(input:string, delim:string)
>  %}
>  
>  /*
> + * strrplc - Takes a parent string, uses the second string which is a
> + *		substring to parent string and replaces that with the
> + *		third string of same lenght as second and returns the
> + *		new converted parent string.
> + * s1	The parent string. If NULL, the function returns NULL.
> + * 
> + * s2	The substring which is used to search in the parent string s1.
> + * 
> + * s3	The substring which is used to replace the searched string s2.
> + *	Both s2 and s3 have to be of same lenght. Else s1 is returned.
> + */

I don't like the restriction that they must be the same length.  I
understand how that makes the implementation easier, but I think it
would be more useful if it allowed arbitrary string lengths.

Also, please choose parameter names that have meaning to what they are.

> +function strrplc:string (s1:string, s2:string, s3:string)
> +%{
> +	static char str[MAXSTRINGLEN];

There's no reason to use a static array here, and it makes this function
SMP-unsafe.  Just build the result directly in the return array.

> +	if(THIS->s1 == NULL)
> +		return THIS->__retvalue = NULL;

Strings in THIS are arrays, not pointers.  They can never be NULL, and
you definitely can't assign them.  This part will give compiler errors.

> +
> +	if(strlen(THIS->s2) != strlen(THIS->s3)) {
> +		strncpy(THIS->__retvalue, THIS->s1, MAXSTRINGLEN);
> +		return;
> +	}
> +
> +	strncpy(str, THIS->s1, MAXSTRINGLEN);
> +
> +	while((ptr = strstr(ptr, THIS->s2)) != NULL) {
> +		memcpy(ptr, THIS->s3, strlen(THIS->s3));
> +		ptr = ptr + strlen(THIS->s3);
> +	}
> +
> +	strncpy(THIS->__retvalue, str, MAXSTRINGLEN);
> +	return;
> +%}

First, I would prefer using strlcpy/strlcat, as they have better
NUL-termination semantics.

To support unequal search and replace string lengths, you can do
something like this pseudocode:

  while search is found:
    strlcat the portion up to the ptr
    strlcat the replace string
  strlcat the tail

Finally, please provide tests for this new function, so we can make sure
that it always works.

Thanks,

Josh