Thread: function that copies token from a string not working

This is a function that is supposed to copy a token from a string into another string. When the second token in a string ie: "cd .." is requested, the start and end pointers point to some gibberish value.

I have stared at this for hours and for the life of me cannot find the problem. Any help would be great, if you need more info let me know.


This functions takes in a string s, a delimiter d, a token number n, and another string for the copy c. It copies the nth token as delimited by d in s to c.

Code:

int cp_delimited_token( char *string, char delimiter, int token_nr,
 			char *token, int max_token )
 {
 	char *start;
 	char *end;
 	int token_len, i;
 
 	// Get token start
 	start = string;
 	for ( i = 0; i < token_nr; i++ ) {
 		if (( start = strchr( string, delimiter )) == NULL ) {
 			return 0;
 		}
 	}
 
 	// Find end of token
 	if (( end = strchr( start, delimiter )) == NULL )
 		end = start + strlen(start);
 
 	token_len = end - start;
 
 	// Check bounds
 	if ( token_len > max_token )
 		token_len = max_token;
 
 	// Copy token, then add '\0'
 	strncpy( token, start, token_len );
 	*(token + token_len + 1 ) = '\0';
 
 	// Replace '\n' with '\0' to end of token
 	for ( i = 0; i < strlen( token ); i++ ) {
 		if ( token[i] == '\n' )
 			token[i] = '\0';
 	}
 	
 	return token_len;
 }

thanks in advance
Mitch

Code:

for ( i = 0; i < token_nr; i++ ) {
 		if (( start = strchr( string, delimiter )) == NULL ) {
 			return 0;
 		}
 	}

You realise that this does nothing other than give you the same token over and over, right?

Code:

#include <stdio.h>
#include <string.h>
int main( void )
{
    char delimiter = ':';
    char s[] = "He:llo: Wo:rl:d!:";
    char *start = s;
    int i, token_nr = 3;

    for ( i = 0; i < token_nr; i++ ) {
        if (( start = strchr( string, delimiter )) == NULL ) {
            return 0;
        }
        printf("start gives us \'%s\'\n", start );
    }
    return 0;
}


start gives us ':llo: Wo:rl:d!:'
start gives us ':llo: Wo:rl:d!:'
start gives us ':llo: Wo:rl:d!:'

You never actually change what start points at.

Code:

// Find end of token
 	if (( end = strchr( start, delimiter )) == NULL )
 		end = start + strlen(start);

Since 'start' points right at the delimiter, strchr on start will just give you the start of start.

See how handy a few printf statements can be for debugging? At any rate, that'll get you headed in the right direction, without me doing everything for you.


Quzah.

The problem is:

Code:

 	start = string;
 	for ( i = 0; i < token_nr; i++ ) {
 		if (( start = strchr( string, delimiter )) == NULL ) {
 			return 0;
 		}
 	}

You're calling strchr() on string every time...you want to be searching after each token. Try chaning the loop to:

Code:

 	start = string;
 	for ( i = 0; i < token_nr; i++, start++ ) {
 		if (( start = strchr( start, delimiter )) == NULL ) {
 			return 0;
 		}
 	}

I just changed strchr() so it's checking start instead of string and then I'm incrementing start at the end of the loop to skip past the delimiter.

Hmm well...I should have refreshed this thread before posting...Sorry quzah, I ended up doing it for him :P

brilliant, thanks heaps guys! Been staring at that code for a long time!

See how handy a few printf statements can be for debugging? At any rate, that'll get you headed in the right direction, without me doing everything for you.

great suggestion will do

I am very happy now!

BTW, your bounds checking is off. You might want to double-check it. If you want to try to fix it on your own then don't look at the following code :

Code:

itsme@dreams:~/C$ cat token.c
#include <stdio.h>
#include <string.h>

int cp_delimited_token(char *string, char delimiter, int token_nr,
                       char *token, int max_token)
{
  char *start;
  char *end;
  int token_len, i;

  // Get token start
  for(start = string, i = 0;i < token_nr;i++, start++ )
  {
    if((start = strchr(start, delimiter)) == NULL)
      return 0;
  }

  // Find end of token
  if((end = strchr(start, delimiter)) == NULL)
    end = start + strlen(start);

  token_len = end - start;

  // Check bounds
  if(token_len >= max_token)
    token_len = max_token - 1;

  // Copy token, then add '\0'
  strncpy(token, start, token_len);
  *(token + token_len) = '\0';

  // Replace '\n' with '\0' to end of token
  for(i = 0;i < strlen(token);i++)
  {
    if(token[i] == '\n')
      token[i] = '\0';
  }

  return token_len;
}

int main(void)
{
  char buf[10];
  int i;

  for(i = 0;i < 3;++i)
  {
    printf("%d: %s\n",
      cp_delimited_token("ab,1234567890,cde", ',', i, buf, sizeof(buf)),
      buf);
  }

  return 0;
}
itsme@dreams:~/C$ ./token
2: ab
9: 123456789
3: cde
itsme@dreams:~/C$

The fact that it cut it off at 9 is correct. 1 of the 10 bytes is reserved for the '\0'.

sweet, thanks I actually put that +1 in whilst trying to fix the first problem to make sure my bounds checking wasnt breaking the code. I had totally forgotton about it so thankyou for pointing that out.