Jump to content


Check out our Community Blogs

Register and join over 40,000 other developers!


Recent Status Updates

View All Updates

Photo
- - - - -

Pointer Seg Fault


  • Please log in to reply
14 replies to this topic

#1 matio

matio

    CC Resident

  • Just Joined
  • PipPipPipPip
  • 50 posts

Posted 22 October 2009 - 07:34 AM

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;

  • 0

#2 marwex89

marwex89

    CC Mentor

  • VIP Member
  • PipPipPipPipPipPipPipPip
  • 2857 posts

Posted 22 October 2009 - 07:38 AM

mstring is not an array, you have only allocated space for a pointer.
  • 0
Hey! Check out my new Toyota keyboaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

#3 matio

matio

    CC Resident

  • Just Joined
  • PipPipPipPip
  • 50 posts

Posted 22 October 2009 - 08:58 AM

So should it be:
char mstring[strlen(string)];
int i = 0;
// snip
mstring[i] = *string;
i++;
?
  • 0

#4 marwex89

marwex89

    CC Mentor

  • VIP Member
  • PipPipPipPipPipPipPipPip
  • 2857 posts

Posted 22 October 2009 - 09:04 AM

Almost. :P 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().
  • 0
Hey! Check out my new Toyota keyboaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

#5 matio

matio

    CC Resident

  • Just Joined
  • PipPipPipPip
  • 50 posts

Posted 22 October 2009 - 09:13 AM

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
  • 0

#6 ZekeDragon

ZekeDragon

    CC Leader

  • Retired Mod
  • PipPipPipPipPipPipPip
  • 1263 posts

Posted 22 October 2009 - 09:32 AM

@marwex89: Don't the C99 standards require the ability to make variable sized local arrays? XD
  • 0
If you enjoy reading this discussion and are thinking about commenting, why not click here to register and start participating in under a minute?

#7 marwex89

marwex89

    CC Mentor

  • VIP Member
  • PipPipPipPipPipPipPipPip
  • 2857 posts

Posted 22 October 2009 - 09:44 AM

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 :D
  • 0
Hey! Check out my new Toyota keyboaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

#8 psam

psam

    CC Regular

  • New Member
  • PipPipPip
  • 35 posts

Posted 22 October 2009 - 12:59 PM

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

  • 0

#9 marwex89

marwex89

    CC Mentor

  • VIP Member
  • PipPipPipPipPipPipPipPip
  • 2857 posts

Posted 22 October 2009 - 01:26 PM

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.

  • 0
Hey! Check out my new Toyota keyboaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

#10 dcs

dcs

    CC Devotee

  • Just Joined
  • PipPipPipPipPipPip
  • 730 posts

Posted 23 October 2009 - 09:44 AM

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.


  • 0

#11 marwex89

marwex89

    CC Mentor

  • VIP Member
  • PipPipPipPipPipPipPipPip
  • 2857 posts

Posted 23 October 2009 - 09:54 AM

:D 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
  • 0
Hey! Check out my new Toyota keyboaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

#12 psam

psam

    CC Regular

  • New Member
  • PipPipPip
  • 35 posts

Posted 24 October 2009 - 05:24 AM

@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;
  • 0




Recommended from our users: Dynamic Network Monitoring from WhatsUp Gold from IPSwitch. Free Download