c language strchr segmentation fault character replace

Question

I need to replace every occurence of '&' to ',' in a C string.

I did this and it works

Code 1:

char *val, *querydup;
.
.
.
val=strchr(querydup,'&');
while(val != NULL) {
*val=',';
val=strchr(querydup,'&');
}

In order to be "elegant" I tried the following, but it leads to seg fault, to the point where even my pointer cursor gets corrupted!. weird, I'm running linux inside a vmware vm.

Code 2:

while(val=strchr(querydup,'&') != NULL) {
*val=',';
}

So what could be wrong?..

Do you consider code 1 to be "elegant"?...

Regards.

I suggest you put `val=strchr(querydup,'&')` in brackets inside the `if` statement. — Omri Barel, Aug 22 '11 at 22:26

score 6 · Answer 1 · edited May 23 '17 at 10:28

6

Your main issue has been answered by emboss.

I'm answering the "elegance" part :)

You're checking the same characters over and over again.

Suppose querydup has "&&&&&&&&&&". After the first few changes it has ",,,,,,&&&&" and yet you still check from the beginning anyway.

Try re-using val for "elegance" :)

val = strchr(val, '&');

edited May 23 '17 at 10:28

Community

1
1

answered Aug 22 '11 at 22:32

pmg

106,608
13
126
198

If you do this, you need to initialise `val` to `querydup`, so a `for()` loop is probably the most elegant: `for (val = querydup; val != NULL; val = strchr(val, '&'))` – caf Aug 22 '11 at 22:54
@caf: assuming `querydup` can never be NULL, I prefer a `do {} while ()` and avoid a redundant test (equivalent to `querydup != NULL`). `val = querydup; do { val = strchr(val, '&'); if (val) *val++ = ','; } while (val); /* trust compiler to only test val once :) */` – pmg Aug 22 '11 at 23:17
@pmg: If you're going to go that way, then what about `val = querydup; while (val = strchr(val, '&'), val != NULL) *val = ',';` – caf Aug 22 '11 at 23:21
That looks good too @caf. You left out the `++` to increase `val` past the new `','` :) Oh ... and I don't like the comma operator and cramming too much work inside the condition. – pmg Aug 22 '11 at 23:25
Personally I would just use the `for()` loop version - the overhead of a single additional test against `NULL` isn't worth worrying about, and in many cases the compiler will be able to elide it anyway. I think you unfairly marginalise the comma operator though ;) – caf Aug 22 '11 at 23:32
val=querydup; while((val=strchr(val,'&')) != NULL) { *val=','; } and worked perfectly, the "for" version looks very good too, and I will use it on newer code. – user905292 Aug 22 '11 at 23:59

score 4 · Answer 2 · edited Jun 08 '12 at 12:33

4

It's the operator precedence, try

while((val=strchr(querydup,'&')) != NULL) {
    *val=',';
}

edited Jun 08 '12 at 12:33

Wladimir Palant

56,865
12
98
126

answered Aug 22 '11 at 22:26

emboss

38,880
7
101
108

score 3 · Answer 3 · answered Aug 22 '11 at 22:28

3

Perhaps a simpler solution would be to actually do it manually:

char *ptr = querydup;
while (*ptr) {
    if (*ptr == '&') {
        *ptr = ',';
    }
    ptr++;
}

It's more lines of code, but it only goes through the string once, where the repeated executions of strchr() will go through the string multiple times.

answered Aug 22 '11 at 22:28

Doug Kress

3,537
1
13
19

Forgot to increment ptr the first time through! – Doug Kress Aug 22 '11 at 22:30
Looks good, however pmg's solution seems even more "elegant" :) thanks!. – user905292 Aug 22 '11 at 22:53

Paul · Answer 4 · 2011-08-22T22:55:38.733

1

You are assigning val a boolean value. Your code is equivalent to:

while(val = (strchr(querydup,'&') != NULL))

Change it to:

while((val = strchr(querydup,'&')) != NULL)

edited Aug 22 '11 at 22:55

answered Aug 22 '11 at 22:28

Paul

139,544
27
275
264

The last one is not valid. The `!=` has higher precedence than `=`, so the left hand side of the `=` operator is `NULL != val`, which is not an lvalue. – caf Aug 22 '11 at 22:52

c language strchr segmentation fault character replace

4 Answers4