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

I like guessing: you passed a "string literal" into your function (which cannot be overwritten).

Also, from the Premature Optimization Division: you may want to store my_strlen(delim) somewhere, rather than computing it in some repeated for loops.

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

Originally Posted by tabstop

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

Originally Posted by tabstop

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

Originally Posted by matsp

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! .

Originally Posted by matsp

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.)

Originally Posted by tabstop

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