14 replies to this topic

[matio]

I have a function that strips a char from a string:

void strip(char *string, char s){
	char *mstring;
	while (*string++ != '\0'){
		if (*string != s){
			*mstring++ = *string;
		}
	}
	copy(string, mstring);
}

when run it gives a seg fault at:

*mstring++ = *string;

[marwex89]

mstring is not an array, you have only allocated space for a pointer.

[matio]

So should it be:

char mstring[strlen(string)];
int i = 0;
// snip
mstring[i] = *string;
i++;

?

[marwex89]

Almost. You got the point, but you will get a compiler error if you try that, since the compiler will not know the size of string (strlen(string)) at compile time. You need to use what is called a dynamic array. Since you're using C:

char* mstring = malloc(strlen(string));
...
free(mstring);

Read up on malloc() and free().

[matio]

Thanks, is there something wrong with the logic? When I run:

void strip(char *string, char s){
	char* mstring = malloc(strlen(string));
	while (*string++ != '\0'){
		if (*string != s){
			*mstring++ = *string;
		}
	}
	copy(string, mstring);
}

like this:

	char string[] = "Hello everybody\n";
	strip(string, '\n');
	printf(string);

it still has a newline

[ZekeDragon]

@marwex89: Don't the C99 standards require the ability to make variable sized local arrays? XD

[marwex89]

Wow, I didn't know that.. The resulting machine code is probably the same, though, and matio should learn the "right" way to do it

[psam]

You really shouldn't do assigments inside conditions. It's just bad practice and makes the code less understandable. Anyway i rewrote your code in a simpler way:

void strip(char* string, char c)
{
    int size=sizeof string;
    char tstring[size];
    int count1=0, count2=0;

    for(count1; string[count1]!='\0'; count1++)
    {
        if(string[count1]!=c)
        {
            tstring[count2]=string[count1];
            count2++;
        }
    }
    tstring[count2]='\0';

    count1=0;
    for(count1; count1<=count2; count1++)
    {
        string[count1]=tstring[count1];
    }
}

[marwex89]

Not to be rude, psam, but

1. You are also doing an assignment inside a condition
2. It is NOT bad practise, wtf...?
3. Your code is less efficient than matio's
4. Your code is - in my opinion - less readable than matio's

Have a nice day.

EDIT: For your problem, matio, your pointer arithmetic is the cause. It's probably better to do it like this:

void strip(char* string, char s)
{
    char* mstring = malloc(strlen(string) + 1);
    int i = 0, a = 0;
    
    while (string[i] != '\0')
    {
        if (string[i] != s)
        {
            mstring[a] = string[i];
            ++a;
        }
        ++i;
    }
    
    mstring[a] = '\0';
    strcpy(string, mstring);
    free(mstring);
}

Edited by marwex89, 23 October 2009 - 09:54 AM.

[dcs]

Question 7.19

The most common source of problems is writing more to a malloc'ed region than it was allocated to hold; a particularly common bug is to malloc(strlen(s)) instead of strlen(s) + 1.

[marwex89]

Ahem... edited.. I actually thought about that one in the shower yesterday, but thought I had fixed it. Would only be a problem if there were no characters to strip

[psam]

@marwex
I didn't write an assigment inside a condition...
I was refering to "while(*string++...".
How can my code be less eficient than his if his code doesn't even work .
Besides, you're code is almost like mine except you use dynamic memory, strcpy instead of a loop and a while instead of a for.

EDIT: Maybe i didn't make myself clear:
1==1; is a condition,
i!=NULL; is a condition,
a=5; is an assigment,
a++; is an assigment,
(a++)!=2; is an assigment inside a condition the same for:
*mystring++!=c;