tags:

views:

406

answers:

6

I know this has been asked thousands of times but I just can't find the error in my code. Could someone kindly point out what I'm doing wrong?

#include <stdlib.h>
#include <string.h>

void reverseString(char *myString){
  char temp;
  int len = strlen(myString);

  char *left = myString;
  //  char *right = &myString[len-1];                                                                                        
  char *right = myString + strlen(myString) - 1;

  while(left < right){
    temp = *left;
    *left = *right; // this line seems to be causing a segfault                                                              
    *right = temp;
    left++;
    right--;
  }
}

int main(void){
  char *somestring = "hello";
  printf("%s\n", somestring);
  reverseString(somestring);

  printf("%s", somestring);

}
+6  A: 

You are invoking Undefined Behavior by trying to modify a potentially read-only memory area (string literals are implicitly const -- it's ok to read them but not to write them). Create a new string and return it, or pass a large enough buffer and write the reversed string to it.

dirkgently
+12  A: 

the problem is here

char *somestring = "hello";

somestring points to the string literal "hello". the C++ standard doesn't gurantee this, but on most machines, this will be read-only data, so you won't be allowed to modify it.

declare it this way instead

char somestring[] = "hello";
John Knoeller
The thing I dislike about this approach is that the function will be modifying its input parameter. YMMV.
dirkgently
dirkgently: Yes, but it's also about twice as fast as the alternative and uses less memory. Some programmers work on tasks where performance matters.
John Knoeller
If the performance critical part of your application is reversing strings in a tight loop I think you should rethink your design.
Mark Byers
@Mark: and you know this how?
John Knoeller
@John: Where would all these string that you have to reverse come from? How can you generate them faster than you can reverse them so that the reversing becomes the bottleneck? I'm sure there are situations where reversing strings is a bottleneck, but they are probably quite rare.
Mark Byers
@John Knoeller: How does modifying the input improve performance? Further, you are loosing the ability to pass string literals as parameter with this approach. And finally, you cannot chain calls (if the return type is `void`).
dirkgently
A: 

Your logic seems correct. Instead of using pointers, it is cleaner to deal with char[].

fastcodejava
+11  A: 

Ultimately, it would be cleaner to reverse it in place, like so:

#include <stdio.h>
#include <string.h>

void
reverse(char *s)
{
    int a, b, c;
    for (b = 0, c = strlen(s) - 1; b < c; b++, c--) { 
        a = s[b]; 
        s[b] = s[c]; 
        s[c] = a; 
    }

    return; 
}

int main(void)
{
    char string[] = "hello";
    printf("%s\n", string);
    reverse(string);
    printf("%s\n", string);

    return 0;
}

Your solution is essentially a semantically larger version of this one. Understand the difference between a pointer and an array. The standard explicitly states that the behviour of such an operation (modification of the contents of a string literal) is undefined. You should also see this excerpt from eskimo:

When you initialize a character array with a string constant:

char string[] = "Hello, world!";

you end up with an array containing the string, and you can modify the array's contents to your heart's content:

string[0] = 'J';

However, it's possible to use string constants (the formal term is string literals) at other places in your code. Since they're arrays, the compiler generates pointers to their first elements when they're used in expressions, as usual. That is, if you say

char *p1 = "Hello";
int len = strlen("world");

it's almost as if you'd said

char internal_string_1[] = "Hello";
char internal_string_2[] = "world";
char *p1 = &internal_string_1[0];
int len = strlen(&internal_string_2[0]);

Here, the arrays named internal_string_1 and internal_string_2 are supposed to suggest the fact that the compiler is actually generating little temporary arrays every time you use a string constant in your code. However, the subtle fact is that the arrays which are ``behind'' the string constants are not necessarily modifiable. In particular, the compiler may store them in read-only-memory. Therefore, if you write

char *p3 = "Hello, world!";
p3[0] = 'J';

your program may crash, because it may try to store a value (in this case, the character 'J') into nonwritable memory.

The moral is that whenever you're building or modifying strings, you have to make sure that the memory you're building or modifying them in is writable. That memory should either be an array you've allocated, or some memory which you've dynamically allocated by the techniques which we'll see in the next chapter. Make sure that no part of your program will ever try to modify a string which is actually one of the unnamed, unwritable arrays which the compiler generated for you in response to one of your string constants. (The only exception is array initialization, because if you write to such an array, you're writing to the array, not to the string literal which you used to initialize the array.) "

Mustapha Isyaku-Rabiu
Thank you for the code and link to explanation! I learned a lot more from this experience than I would cared to imagine :P
qwer
@qwer: Glad to help.
Mustapha Isyaku-Rabiu
Kickass answer.
Daniel Goldberg
Nice answer. +1.
Alok
A: 
Dear Friend,

 You can use the following code 


#include<stdio.h>
#include<string.h>
#include<malloc.h>
char * reverse(char*);

int main()
{
        char* string = "hello";
        printf("The reverse string is : %s", reverse(string));
        return 0;
}

char * reverse(char* string)
{

   int var=strlen(string)-1;
     int i,k;
     char *array;
     array=malloc(100);
     for(i=var,k=0;i>=0;i--)
    {
           array[k]=string[i];
            k++;
   }
  return array;
}
muruga
A: 

I take it calling strrev() is out of the question?

Martin Milan
Yep, the point was to try writing it myself.
qwer