Try to reverse Line with pointers in C

Question

I get error when I use pointer to reverse function , when I try to call to the reverse function, and than to print it. pointer send error message . So i was tried to get change some code , but it helpless.

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

char *strrev(char *str)
{
    char *p1, *p2;

    if(!str) {return NULL;}
    printf("%s",__LINE__); //checking
    for (p1 = str, p2 = str + strlen(str) - 1; p2 > p1; ++p1, --p2)
    {
        *p1 ^= *p2;
        *p2 ^= *p1;
        *p1 ^= *p2;
    }
    printf("%s",str);  //checking
    return str;
}

int main(void)
{
    char *str ;
    printf(" Please enter string \n");
    scanf("%s",&str);
    printf("%s\n", strrev(*str));
    return 0;
}

Sharing what the error message would be useful too. For all we know it's "compiler doesn't like your hair color". — Marc B, May 07 '12 at 03:20
The xor code to swap values is cute but not particularly sensible. Use a simple swap: `char t = *p1; *p1 = *p2; *p2 = t;` instead. Also, calling `printf()` when `strrev()` can return a NULL pointer is not reliable. Some systems will handle a NULL OK; others will not, and are not required to by the C standard. — Jonathan Leffler, May 07 '12 at 03:28
@Marc, there is _no_ mention of hair-colour-defined behaviour in the ISO C standard. This sort of misinformation doesn't do _anyone_ any favours :-) — paxdiablo, May 07 '12 at 03:42
Also, `printf("%s",__LINE__);`... I thought `__LINE__` evaluated to an integer not a string... — dreamlax, May 07 '12 at 03:52

score 4 · Answer 1 · answered May 07 '12 at 03:21

4

char *str ;
scanf("%s", &str);

You never allocate memory for str. Try malloc or better yet, use an array in the first place
You need to drop the & from the scanf line

answered May 07 '12 at 03:21

cnicutar

178,505
25
365
392

so what I gonna do.. catch memory by malloc() or change to array?? – Dima Shifer May 07 '12 at 03:25
2

@DimaShifer: you can do either; changing to an array is simpler. – Jonathan Leffler May 07 '12 at 03:27
Yes, you definitely need to allocate some memory, eg. `char str[256];`. However, using `scanf` is dangerous, as it does not check the length of the receiving buffer and thus could overflow the receiver. A safer alternative is to use `fgets()`, which takes a length parameter for the string. – gavinb May 07 '12 at 03:38
@gavinb You can try `scanf("%255s", str)` to make it safer. – cnicutar May 07 '12 at 03:39

score 2 · Answer 2 · edited May 23 '17 at 12:11

You have quite a few issues there.

1/ First, you do not allocate any storage for your string. This could be fixed with a simple char str[128]; depending on your size requirements. This will also mean that the NULL check is unnecessary and you don't have to worry about later printing (or trying to print) a NULL pointer - change the function contract so that this is explicitly forbidden.

2/ Next, don't ever use scanf("%s") (ie., with an unbounded string format specifier) - it's an easy pathway to buffer overflow problems. If you want a robust input function with overflow checking see this answer.

In any case, %s scans a word rather than a line so, if you enter "Hello Pax" (with a space), it will see Hello.

3/ Thirdly, if str is a char*, then *str is a character, not a C string. You should be passing str to strrev, not *str. This is true even with the fix in point 1 above.

4/ The __LINE__ macro expands to an integral type so won't take kindly to being treated as a character pointer as per the %s format specifier. You may want to use %d instead.

5/ The ISO standard reserves all names starting with str and a lowercase letter for its own uses, as per 7.31.13 Future library directions in C11. So strrev is not actually a good idea.

6/ Lastly, the XOR-swap trick is an anachronism that has no place in today's modern environments. It's both unnecessary and may well be slower, given the calculations rather than simple moves of a temporary-variable solution. The correct way to swap to variables is:

int t = a;
a = b;
b = t;

Do yourself a favour and ditch that trick. It belongs in the same bucket as Duff's Device :-)

Some other issues, though this are more stylistic than anything else (as in I prefer this style), based on quite a few years of teaching programming.

7/ Most people I've encountered find it easier to understand array indexes rather than pointers since it's easier to think about a character in a string rather than an arbitrary memory location. Seasoned C programmers know there's no difference (both in terms of functionality and, with today's compilers, performance). Hence I tend to use indexes rather than pointers, such as with:

char *reverseStr (char *str) {
    char tmpchar;
    int front = 0;
    int back = strlen (str) - 1;
    while (front < back) {
        tmpchar = str[front];
        str[front++] = str[back];
        str[back--] = tmpchar;
    }
    return str;
}

8/ You'll also notice another thing there, the use of more verbose variables like front, back and tmpchar. The compiler doesn't care how big your identifiers are (within reason) and this makes code much more readable.

Thanks a lot I'll try to do some changes))) – Dima Shifer May 07 '12 at 04:24 — Dima Shifer, May 07 '12 at 04:24

Try to reverse Line with pointers in C

2 Answers2