This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
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