Unexpected output when I try to reverse a char*

Question

#include<iostream>
#include<string>
using namespace std;

void reverse(char* str)
{
    char *new_str = str;
    while(*new_str != '\n'){
        new_str++;
    }
    while(new_str != str){
        cout << *new_str;
        new_str--;
    }
    cout << *new_str;
}

int main()
{
    char *str = new char[1024];
    str = "hello world";

    reverse(str);
}

When I try to run this I get some crazy output and my computer starts to beep. What am I doing blatantly wrong here?

score 4 · Answer 1 · edited May 23 '17 at 12:03

4

The end of a C string is marked by the character '\0'. You used '\n' which is the newline character.

edited May 23 '17 at 12:03

Community

1
1

answered Jan 10 '14 at 11:58

slaphappy

6,894
3
34
59

So simple. Thank you! Do you know why I get a warning telling me "deprecated conversion from string constant to 'char*'." I know it's because the line str = "hello world"; but is this just a warning I can ignore? – user3150601 Jan 10 '14 at 12:00
1

Yes, since you use a string constant, you should use `const char *` instead of `char *`. The `const` indicates that the data pointed to is constant. – slaphappy Jan 10 '14 at 12:01
Also, the two first lines in `main()` should be replaced by `const char *str = "hello world";`. There's no need to allocate for the string constant, the compiler does it for you. – slaphappy Jan 10 '14 at 12:02
2

This is also a memory leak as the new'd array pointer is lost (+ never used). – dornhege Jan 10 '14 at 12:12

TemplateRex · Answer 2 · 2014-01-10T12:42:54.897

2

You mean apart from using the naked leaky new, the deprecated char* instead of const char* or even better std::string, not using a Standard Library algorithm std::reverse, mixing IO with your algorithm and including the entire namespace std (which might indirectly bring std::reverse() into scope) without putting your own reverse() inside its own namespace?

#include <algorithm>
#include <iostream>
#include <iterator>
#include <string>

// using namespace std; // for the brave, and drop the std:: in the next 3 lines

int main()
{
    std::string str = "hello world";    // std::string instead of char*
    std::reverse(begin(str), end(str)); // standard library algorithm
    std::cout << str;                   // IO separate from algorithm
}

If you are only interested in how to code a reverse algorithm, here is one way to do it without relying on the fact that you have a null terminator:

template<class BidirIt>
void reverse(BidirIt first, BidirIt last)
{
    while ((first != last) && (first != --last)) {
        std::swap(*first++, *last);
    }
}

edited Jan 10 '14 at 12:42

answered Jan 10 '14 at 12:02

TemplateRex

69,038
19
164
304

@DarkWanderer don't get me started: define your own `reverse()` function and if by accident `` also includes ``, then you open yourself up to nasty ADL errors. – TemplateRex Jan 10 '14 at 12:05
I know how to use the STL. The reversed char* is an interview-type question so your response doesn't help me. So thanks, but no thanks. – user3150601 Jan 10 '14 at 12:05
@user3150601 You didn't give that context. But even then: you could still use the same signature as `std::reverse`, taking two iterators instead of relying on the implicit null terminator. E.g. what if you were asked to reverse a `std::vector`, then you would have to change your entire algorithm. You could also still separate IO from the algorithm itself. Emulating the STL is still better practice than what you posted (no offense, but you asked what was blatantly wrong, and that code should never pass code review). – TemplateRex Jan 10 '14 at 12:07
So is what I posted technically incorrect? Like is this not what an interviewer would expect to see? I can't imagine they're looking for the pre-defined reverse(). – user3150601 Jan 10 '14 at 12:11
@user3150601 OK, I see your point. I posted a few lines with a `reverse` that I would expect as an interviewer. – TemplateRex Jan 10 '14 at 12:13
Look at the first sentence in this answer + the comments. There are couple of problems with this code, i.e. possible segfaults, memory leaks, mixing output with processing, etc. – dornhege Jan 10 '14 at 12:13
Honestly, if I were an interviewer and would ask: How do you reverse a string: `std::reverse` is the best answer, because that's what I'd like my devs to do. Everything else should be introduced like: "As an exercise, how would you do that manually?" – dornhege Jan 10 '14 at 12:15
@TemplateRex: C++ 101: don't define names which might clash with keywords/STL. Not a place to start a holywar, but suffice to say this specific point is your personal opinion rather than universal "best practice". – DarkWanderer Jan 10 '14 at 12:15
@DarkWanderer ehm, of course you can define your own `reverse`, but always in your own namespace and don't use `using namespace std` at the same time and expect to get away with it. – TemplateRex Jan 10 '14 at 12:16
@TemplateRex: somehow I do always get away with it - never had a single problem with name clash. OTOH, readability with std:: everywhere is terrible. I'm not even speaking of this awful C# code with tens of namespaces before each class name, created by adepts of the same practice. – DarkWanderer Jan 10 '14 at 12:21
@DarkWanderer in this case the use of `std::` is in 3 places (you can rely on ADL to find `begin()` and `end()`). I don't find that excessive. YMMV of course. – TemplateRex Jan 10 '14 at 12:23
Well, your comment was obviously about general practice, not just your code - so are mine. In prod code it's not "3 places". – DarkWanderer Jan 10 '14 at 12:31
@DarkWanderer I qualified my remark about `using namespace std` with the context that it might introduce a name conflict if a user-defined `reverse()` was not put in its own namespace. I didn't meant to start a general debate on this issue, or sell my own opinion as general practice. – TemplateRex Jan 10 '14 at 12:36

Vlad from Moscow · Answer 3 · 2014-01-10T12:48:41.147

The problem is that at first you assigned str the address of allocated memory and then reassigned it to point to string literal that has type const char[] in C++.

char *str = new char[1024];
str = "hello world";

This string literal has terminating zero char '\0'. It has no the new line char '\n'. So the function is invalid because it will try to access memory beyond the array searching the new line char.

The valid code could look the following way

#include <iostream>
using namespace std;

void reverse( const char* s )
{
    const char *p = s;

    while ( *p ) p++;

    while ( p != s ) cout << *--p;
}

int main()
{
    const char *s = "hello world";

    reverse( s );
}

Or if you want to enter a string yourself interactively then main could look as

int main()
{
    const size_t N = 1024;
    char s[N];

    cout << "Enter a statement: ";
    cin.getline( s, N );

    reverse( s );
}

And `reverse` should be renamed (`print_reverse` ?) as it doesn't reverse anything. — Jarod42, Jan 10 '14 at 13:29
@Jarod42 I think it is unimportant for this simple exercise. Though of couse name reverse can confuse the user. — Vlad from Moscow, Jan 10 '14 at 13:31

Jai · Answer 4 · 2014-01-13T05:41:13.760

0

correct your function :

void reverse(char* str)
{
    char *new_str = str;   
    while(*new_str){ // use this instead of *new_ptr != '\n'
        new_str++;
    }
    while(new_str != str){
        cout << *new_str;
        new_str--;
    }
    cout << *new_str;
}

edited Jan 13 '14 at 05:41

answered Jan 10 '14 at 13:01

Jai

1,292
4
21
41

Unexpected output when I try to reverse a char*

4 Answers4