Thread: help implementing strtok()

  1. #1
    Registered User
    Join Date
    Oct 2007
    Posts
    100

    help implementing strtok()

    hello there!
    I wanted to write my own implementation of strtok(), which I call my_strtok().. here is my code:

    Code:
    char *token;
    char* my_strtok(char *str, const char *delim)
    {
    	
    	int i;
    	size_t j;
    	char *string;
    
    	if (str!=NULL) 	{
    		string=str;
    	}
    	else 
    	{
    		string=token;
    	}
    
    	if (my_strlen(string)==0)
    		return NULL;
    
    	int end=0;
    	i=0;
    
    	while (!end)
    	{
    		if (string[i]=='\0')
    		{
    			token=&string[i];
    			
    			return NULL;
    		}
    
    		for (j=0;j<my_strlen(delim);j++)
    		{
    			if (string[i]==delim[j])
    			{
    				i++;
    				end=0;
    				break;
    			}
    			else
    			{
    				end=1;
    			}
    		}
    	
    	}
    	
    	char *tok;
    	tok=&string[i];
    	i++;
    	end=0;
    	while (!end)
    	{
    		if (string[i]=='\0')
    		{
    			token=&string[i];
    			return tok;
    		}
    
    		for (j=0;j<my_strlen(delim);j++)
    		{
    			if (string[i]==delim[j])
    			{
    				
    				end=1;
    				break;
    			}
    		}
    		i++;
    	}
    	string[i-1]='\0';
    	token=&string[i];
    	return tok;
    	
    }
    I found out that
    Code:
    	string[i-1]='\0';
    causes a segmentation fault... Maybe at the moment I am mentally tired due to a long exercise session, but I cannot find an explaination at a glance...
    What do you think in general of this implementation?

    thanks a lot! bye

  2. #2
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,336
    I like guessing: you passed a "string literal" into your function (which cannot be overwritten).

  3. #3
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,336
    Also, from the Premature Optimization Division: you may want to store my_strlen(delim) somewhere, rather than computing it in some repeated for loops.

  4. #4
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    I think tabstop is right. Either that, or i-1 is less than zero.

    Code:
    char *token;
    Make that a static local variable (or at the very least a static global) - it avoids someone writing the following getting nasty surprises:

    Code:
    char *token;
    
    void myfunc()
    {
      ...
      token = mystrtok(...);
      ...
    }
    Unless you strings are very different from standard C strings, perhaps you can replace this:
    Code:
    	if (my_strlen(string)==0)
    with a simple check to see
    Code:
    if (string[0] == 0) ...
    On the other hand:
    Code:
    		if (string[i]=='\0')
    		{
    			token=&string[i];
    			
    			return NULL;
    		}
    will do the same thing for i = 0, aside from setting token to the last character in the string - which I think is pretty meaningless, as you'll never find anything at that point, so token can be set to NULL just as well.

    Code:
    for (j=0;j<my_strlen(delim);j++)
    NEVER EVER call strlen() as a loop control [unless the string length changes during the code of course] - make a local variable of the length before the loop and compare with that.

    Some places, you have inconsistent use of empty lines (e.g. the return NULL code above is different from the otherwise identical bit in the second while-loop.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  5. #5
    Registered User
    Join Date
    Oct 2007
    Posts
    100
    Quote Originally Posted by tabstop View Post
    I like guessing: you passed a "string literal" into your function (which cannot be overwritten).
    yes it's true i passed
    Code:
    char *str=".;aaa...bb;;;;ccc.;.;";
    and it didn't work even with the library strtok!
    now with
    Code:
    char str[]=".;aaa...bb;;;;ccc.;.;";
    it seems to work!

    thanks

  6. #6
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by tabstop View Post
    Also, from the Premature Optimization Division: you may want to store my_strlen(delim) somewhere, rather than computing it in some repeated for loops.
    That's not premature optimization, it's avoiding obvious pessimisation. Premature optimization is when you optimize code that has no need to be optimized or write overly complicated code in an attempt to make it faster, when simple code would do. Storing the length of a potentially rather long string and not comparing it EVERY SINGLE loop iteration is not premature optimization, it's avoiding obvious pessimisation.

    it's not like a single varibel holding the length of the delimiter string is going ot increase the complexity of the code (much).

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  7. #7
    Registered User
    Join Date
    Oct 2007
    Posts
    100
    Quote Originally Posted by matsp View Post
    I think tabstop is right. Either that, or i-1 is less than zero.

    Code:
    char *token;
    Make that a static local variable (or at the very least a static global) - it avoids someone writing the following getting nasty surprises:

    Code:
    char *token;
    
    void myfunc()
    {
      ...
      token = mystrtok(...);
      ...
    }
    Unless you strings are very different from standard C strings, perhaps you can replace this:
    Code:
    	if (my_strlen(string)==0)
    with a simple check to see
    Code:
    if (string[0] == 0) ...
    On the other hand:
    Code:
    		if (string[i]=='\0')
    		{
    			token=&string[i];
    			
    			return NULL;
    		}
    will do the same thing for i = 0, aside from setting token to the last character in the string - which I think is pretty meaningless, as you'll never find anything at that point, so token can be set to NULL just as well.

    Code:
    for (j=0;j<my_strlen(delim);j++)
    NEVER EVER call strlen() as a loop control [unless the string length changes during the code of course] - make a local variable of the length before the loop and compare with that.

    Some places, you have inconsistent use of empty lines (e.g. the return NULL code above is different from the otherwise identical bit in the second while-loop.

    --
    Mats
    thanks also for these suggestions! .

  8. #8
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,336
    Quote Originally Posted by matsp View Post
    That's not premature optimization, it's avoiding obvious pessimisation. Premature optimization is when you optimize code that has no need to be optimized or write overly complicated code in an attempt to make it faster, when simple code would do. Storing the length of a potentially rather long string and not comparing it EVERY SINGLE loop iteration is not premature optimization, it's avoiding obvious pessimisation.

    it's not like a single varibel holding the length of the delimiter string is going ot increase the complexity of the code (much).

    --
    Mats
    Right. (My admittedly weak point was that that was not going to fix whatever the problem was.)

  9. #9
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by tabstop View Post
    Right. (My admittedly weak point was that that was not going to fix whatever the problem was.)
    Sure, but it does respond to
    What do you think in general of this implementation?
    in the original post. Not that cboard is particularly good at not giving unsolicitated advice in general anyways... ;-)

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. 20q game problems
    By Nexus-ZERO in forum C Programming
    Replies: 24
    Last Post: 12-17-2008, 05:48 PM
  2. strtok is causing segmentation fault
    By yougene in forum C Programming
    Replies: 11
    Last Post: 03-08-2008, 10:32 AM
  3. trying to use strtok() function to parse CL
    By ohaqqi in forum C Programming
    Replies: 15
    Last Post: 07-01-2007, 09:38 PM
  4. Help debugging my program
    By shoobsie in forum C Programming
    Replies: 4
    Last Post: 07-05-2005, 07:14 AM
  5. Trouble with strtok()
    By BianConiglio in forum C Programming
    Replies: 2
    Last Post: 05-08-2004, 06:56 PM