This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add Strrplc() To Tapsets


On Tuesday 16 Jun 2009 2:01:10 am Josh Stone wrote:
> 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()?
>
i think str_replace() seems to a good name.
> > ---
> >  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.
>
Well, its not because of the implementation but my usecase when i wrote this 
didnt need variable length replace string. I think its a good idea to provide 
it. I will implement it in next patch.
> 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.
>
ok, will fix that.
> > +	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.
>
Sorry, i just added the check in the last min before creating the patch and 
forgot to test it. Will fix it.
> > +
> > +	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.
>
ok
> 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.
>
Yes i will submit testcase
> Thanks,
>
> Josh

Regards,
Varun
-- 
Regards,
Varun Chandramohan


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]