parsing function riddle

[ahoodin]

There is some kind of problem with this generic function I have written. The purpose of this function is to get at one of the fields in a comma (or other) delimeted string. cSrc is the delimeted line, cRet will be the field, nSubstr represents which field to grab and cDelimeter will be some delimeter. I realize there is a 1k string size limit.

(VC6 W2000 PII 866MHz)


int parse(char *cSrc, char *cRet, int nSubstr, char* cDelimeter)
{
//can use negative number to get last string in block
int nRetval = 0, nstrCtr=0;
char cTemp[1024]="", string[1024]="";
char *token= string;
strcpy(cTemp,cSrc);
token = strtok( cTemp, cDelimeter );
while( token != NULL)
{
if ( strlen(token) >0 && token !=NULL ) nstrCtr++;
if (nstrCtr == nSubstr && token !=NULL) strcpy(cRet,token);
token = strtok( NULL, cDelimeter );
nRetval = nstrCtr;
}
if ((token == NULL) && (nSubstr < 0)) nRetval = nstrCtr;
return nRetval;
}


When I use it to parse a lot of text I get blinking controls, so I am thinking that there may be some kind of memory access violation that is occuring and being handled gracefully in debug mode. There no obvious memory error, however maybe someone has some ideas.

What could the problem with this function be? (beside the way that it is indented , sorry just dealing with the editor here)

-ahoodin

I would like this function not to raise many exceptions and if there is a possible problem to try to fix it without raising additional errors or messageboxes.

[Philip Nicoletti]

Off hand, I don't see any memory problems (assuming
the input cSrc is not too big).

A few notes :

1) you don't really need the "string" variable.

2) the cTemp variable is not really needed (unless you
don't want to modify the original cSrc variable).

3) you mentioned : //can use negative number to get last string in block

I assume that this means nSubStr can be negative. If that
is the case, you never copy anything into cRet.

4) is it possble to determine the input to this function that
causes the controls to start blinking ?

[ahoodin]

>1) you don't really need the "string" variable.
thanks, I see what you mean. The strtok function returns a pointer to char, which means the memory has already been allocated.

>2) the cTemp variable is not really needed (unless you
>don't want to modify the original cSrc variable).
strtok actually replaces the delimeters with NULLs in the string and I dont want to do that.

>3) you mentioned : //can use negative number to get last string >in block I assume that this means nSubStr can be negative. If >that is the case, you never copy anything into cRet.
correct, this is just a case to find out how many fields are in the line.

>4) is it possble to determine the input to this function that
>causes the controls to start blinking ?

I think it may be a NULL in the cSrc.

I will have to start pumping crazy values into the function.

Thanks alot for your feedback Phillip, it was helpful!

More is appreciated very much!

Here is the new but not entirely fixed function:


int parse(char *cSrc, char *cRet, int nSubstr, char* cDelimeter){//can use negative number to get last string in block
int nRetval = 0, nstrCtr=0;
char cTemp[1024]="";
char *token;
strcpy(cTemp,cSrc);
token = strtok( cTemp, cDelimeter );
while( token != NULL)
{ if ( strlen(token) >0 && token !=NULL ) nstrCtr++;
if (nstrCtr == nSubstr && token !=NULL) strcpy(cRet,token);
nRetval = nstrCtr;
token = strtok( NULL, cDelimeter );
}
if ((token == NULL) && (nSubstr < 0)) nRetval = nstrCtr;
return nRetval;
}

[Bob Davis]

Looks like I beat the gurus to it on this one.

Why not use std::string instead of your C-style strings? The fact that you were talking about exceptions indicates that you're using C++, so I think using std::string could simplify your code a bit.

First of all, it would eliminate your 1k character limit, since strings size themselves. Secondly, you could use std::string::find to look for the delimiters. This would eliminate your need for cTemp, because find() doesn't do anything nasty to the string like strtok() does.

This is probably a moot point, but you could try checking out boost.org and see if their boost::tokenizer class meets your needs. Many of those classes are up for adoption in the next revision of the standard, so you know they're well written.

[ahoodin]

Not a terrible Idea Bob, even though the entire application is not namespace std, that doesnt mean I cant just use std::mystring rather than: "using namespace std;".

Also if I wanted to stop the 1k barrier, I could probably use new operator.

I wonder which has higher overhead, namespace std or cTemp?

Probably cTemp if it is a multithreaded application.

Could also probably benefit from learning templates better.

ahoodin

[ahoodin]

Yep I thought about it.

Will take a while to do that, and learn all that stuff. Gotta fix this now. Please look at my problem as it is, like Phillip did.

Thank you,

ahoodin

[Bob Davis]

Here's a quick untested version of your function using std::string:

#include <string>

std::string parse(const std::string &source, const int whichSubString, const std::string &delimiter)
{
     std::string:size_type start, end;
     
     start = source.begin();
     for (int i = 0; i < whichSubString; i++)
     {
          start = source.find(delimiter, start);
          if (start == std::string::npos) // no more delimiters!
               return "";
     }

     end = source.find(delimiter, start);
     if (end == std::string::npos) // there was no delimiter after the token you want; it must be the last token.
          end = source.end();
     
     return source.substr(start, end - start);
}

Again, I've not tested this code, so it could be flawed in some way, but hopefully it gets you on the right track.