access violation in this function? 2

[ahoodin]

I use this function to parse delimeted data. When it is used in a loop sometimes I wonder about it. Normally solid dialogs will blink a bit when using data returned from it. Can anyone see anything?

TIA,

ahoodin


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;
}

 

[Kocky]

The first line in your code is:

int parse(char *cSrc, char *cRet, int nSubstr, char* cDelimeter)

This should be:

int parse(char *cSrc, char *cRet, int nSubstr, char * cDelimeter)

You wrote: char* cDelimeter
This should be: char *cDelimeter

 

[ahoodin]

I dont think that is the access violation.

equivalently:

This should be: char *cDelimeter
this should really be:
This should be: char *cDelimeter

Two spaces after any colon, Thanks.

Seriously lets not fight over it, it's pointless and borin.

ahoodin

 

[Salem]

> When it is used in a loop sometimes I wonder about it
Well if you're calling it in a loop like

for ( i = 0 ; i < n ; i++ ) {
  parse( string, result, i, delims );
}

then yeah, its very expensive - especially if the strings are long. You tokenise the whole string just to extract one token.
It would be better to create an array of tokens and tokenise the whole string just once.

There's also a lot of redundant code in there as well.
>

  if ( strlen(token) >0 && token !=NULL )

This is always true because;
- strtok(ptr,&quot;;&quot sees the &quot;;;&quot; pair in &quot;hello;;world&quot; as a single token, so if it finds anything at all, the substring will have a length > 0.
- the while loop ensures that token will be != NULL anyway.

If there were any real chance of token being NULL, then you would write it like this

  if ( token != NULL && strlen(token) > 0 )

to protect the strlen() function from operating on the NULL pointer.

>

  if ((token == NULL) && (nSubstr < 0)) nRetval = nstrCtr;

Again, the only way of the while loop is for token to be NULL, so the first part of the conditional is always true.
Also, the assignment is always performed inside the loop anyway, so I can't see the point of repeating it again outside the loop.

>

char cTemp[1024]="";

This initialises all 1024 elements of the string to 0. This is a waste of time since you immediately follow it with a strcpy(). Ideally, the thing you should be checking is that cTemp is actually big enough for the string being copied into it.

--

http://www.catb.org/~esr/faqs/smart-questions.html

 

[ahoodin]

ty very much salem, good points.

here is a new function based on a previous post of yours i believe. I added a little that I think works great.

ahoodin

void find_field ( char *line, char* cRet, char delim, int count ) {
char *p = line;
char *ep;
int i,imax;
for ( i = 0 ; i < count && p != NULL ; i++ ) {
p = strchr( p, delim ); /* find it */
if ( p != NULL ) p++; /* skip it */
}
if ((ep=strchr(p,delim)) != NULL)
{
i = 0;
imax = ep-p;
while (i++ < imax) *(cRet++) = *(p++);
//cRet=p[i++];
cRet[imax]=NULL;
}
}

 

[Salem]

Seems OK, except I would use strncpy instead of a while loop

int imax = ep - p;
strncpy( cRet, p, imax );
cRet[imax] = '\0';

> if ((ep=strchr(p,delim)) != NULL)
This needs to be guarded against p being NULL as well

if ( p != NULL && (ep=strchr(p,delim)) != NULL)

With this in mind, you probably want to initialise the return string to be the empty string, just in case nothing is found

*cRet = '\0';

Finally, when posting code, it's preferable to use [ignore]

[/ignore] tags to surround the code in question. It stops

[i]

subscripted arrays from turning the rest of the post into italics

--

http://www.catb.org/~esr/faqs/smart-questions.html

 

[ahoodin]

>Seems OK, except I would use strncpy instead of a while >loop

Ok, I understand, but I like the while loop. Cant think of a reason not to do it.

>This needs to be guarded against p being NULL as well

I think if p == NULL but ep != NULL then ep could be set to p+strlen(p). That way the last field could be ;mydata\0 instead of having to be ;mydata;\0.

I have a larger question now. My other routine will allow the coder to have multiple delimeters, such as &quot;[]&quot; will return fields delimeted with [ and ] or ( and ). How could i implement that? maybe this is where strtok() starts to add up to other methods.

ahoodin

 

[Salem]

> I think if p == NULL but ep != NULL then ep could be set to p+strlen(p).
No you can't - if p is NULL, there is no strlen(p)

> My other routine will allow the coder to have multiple delimeters, such as &quot;[]&quot;
The multi-char equivalents of strchr() are

size_t strspn(const char *s, const char *accept);
size_t strcspn(const char *s, const char *reject);

But watch out - they return counts of chars, not pointers to chars.

--

http://www.catb.org/~esr/faqs/smart-questions.html

 

[ahoodin]

>No you can't - if p is NULL, there is no strlen(p)
Sorry I meant if ep==NULL then but p!=NULL then that could be the last field.

>size_t strspn(const char *s, const char *accept);
>size_t strcspn(const char *s, const char *reject);
I will look at those now.


>But watch out - they return counts of chars, not pointers >to chars.
thanks for the tip.

ahoodin

 

[bodhi]

> char cTemp[1024]=&quot;&quot;;
> This initialises all 1024 elements of the string to 0.

Interesting - assume this is compiler dependant ??

Certainly mine (Symantec /Digital Mars) initialises only the first element of the array.

Does anyone know if the ANSI standard specifies 'correct' behaviour/implementation here ?

 

[Salem]

> Interesting - assume this is compiler dependant ??
Not if it claims to be ANSI-C it shouldn't be.

To quote from the draft C99 standard (

http://anubis.dkuug.dk/jtc1/sc22/wg14/www/docs/n869/),

it says the following
[tt]
6.7.8 Initialization
.....snip.....
21 If there are fewer initializers in a brace-enclosed list than there are elements or members
of an aggregate, or fewer characters in a string literal used to initialize an array of known
size than there are elements in the array, the remainder of the aggregate shall be
initialized implicitly the same as objects that have static storage duration.
[/tt]
Since

""

is the same as

{ '\0' }

, the remaining elements of the array are implicitly initialised to zero also (as they would if the array were static).

This is true of all initialised aggregates (arrays, structures, unions) - any trailing items which are unspecified are defaulted to zeros (of the appropriate type).

A quick look at the assembly code produced by gcc shows the following loop initialising the array

	movl	$1023, %ecx
	movb	$0, %al
	rep
	stosb

--

http://www.catb.org/~esr/faqs/smart-questions.html

 

[bodhi]

Good to know ... thanks.

 

[Kocky]

No man am not talking about two spaces !
You wrote in your function declaration:

char* cDelimeter

Try the following: char *cDelimeter

char* cDelimeter is a pointer to a single character.

char *cDelimeter is a pointer to an arry of characters.

 

[ahoodin]

>No man am not talking about two spaces !
>You wrote in your function declaration:
>char* cDelimeter

while semantically you may be right and this may mean pointer to char.

>Try the following: char *cDelimeter

and this may be pointer to an array of characters

if I increment *cDelimeter++ in either case, I still get the next element.

If your point is that it is less clear to write char* cDelimeter when there is going to be the memory address of an array of characters passed into the function instead of a pointer to a single char then you are right.

 

[Kocky]

Are you sure that the variable &quot;char *cSrc&quot; which enters
the function is null-terminated? If not than the strcpy function is unpredictable.

 

[PerFnurt]

>Are you sure that the variable &quot;char *cSrc&quot; which enters
the function is null-terminated? If not than the strcpy function is unpredictable

By applying Murphys Law to such situation I predict it will crasch the app, not just cause the dialogs to &quot;blink a bit&quot;.

Just to clarify:
The compiler sees no difference between
char* foo;
or
char * foo;
or
char *foo;
or
char
/* Bingely */*/* Beep */ foo;


Only humans or humanoids see the difference. Which to use is a matter of aesthetics, personal preference and what makes most sense for the situation.

/Per
[sub]
if (typos) cout << &quot;My fingers are faster than my brain. Sorry for the typos.&quot;;
[/sub]

 

[ahoodin]

PerFnurt,

I looked in K & R Vol2, and it did not cover this subject. Thank you for clearing that up. I ran some tests, and found no functional differences.

 

[PerFnurt]

I forgot one item in my list of reasons to choose one or the other: Clarity.

For instance, consider this:
char* foo, bar;

I bet that one (talking about humans/humanoids) when least expecting it, at a quick glance, on a bad day, being delayed and hunted by a project manager with a whip, will interpret it as:
char *foo, *bar;

I dont think
char *foo, bar;
would be as easy to misinterpret, and neither would:
char* foo; /* or char *foo; */
char bar;

...and now we're back on the personal preferences.


/Per
[sub]
if (typos) cout << &quot;My fingers are faster than my brain. Sorry for the typos.&quot;;
[/sub]

 

[ahoodin]

Well,

If you are learning C and you are typing:

char* foo, bar;

You are makeing a fundamental mistake, because
bar is now type char and foo is type pointer to char.

However if you look at this line:

parse(char *cSrc, char *cRet, int nSubstr, char* cDelimeter)

You see that the first two instances do have the indirection operator at the correct spot. What we have is a typo in the parameter list of a function definition. Whoops, I see it, but should I loose sleep over it?

We do not have a case where we are declaring varaibles in this fashion, so we should assume that this is a typo instead of lack of fundamental understanding.

If your point is that this typo could manifest its self in the form of an error later in life, you are correct, but we all make typos every day and they manifest themselves in many different ways.

dont loose sleep, it is just a typo.

ahoodin

 

[PerFnurt]

>If your point is that this typo could manifest its self in the form of an error later in life, you are correct, but we all make typos every day and they manifest themselves in many different ways.

Yes. And the sonner they manifest themselves the better:
At best at compile time - at worst at the customer who bought the program.

I'm not losing sleep over it, I just do my best (I do it whenever I get the chance) in promoting the writing of clear and unobfuscated code on several levels.
This example, with the placement of *, is prolly one of the lowest levels...



/Per
[sub]
if (typos) cout << &quot;My fingers are faster than my brain. Sorry for the typos.&quot;;
[/sub]