tags:

views:

324

answers:

7

This is a quick program I just wrote up to see if I even remembered how to start a c++ program from scratch. It's just reversing a string (in place), and looks generally correct to me. Why doesn't this work?

#include <iostream>
using namespace std;

void strReverse(char *original)
{
    char temp;
    int i;
    int j;
    for (i = 0, j = strlen(original) - 1; i < j; i++, j--)
    {
     temp = original[i];
     original[i] = original[j];
     original[j] = temp;
    }
}

void main()
{
    char *someString = "Hi there, I'm bad at this.";
    strReverse(someString);

}
+2  A: 

You're trying to modify a string literal - a string allocated in static storage. That's undefiend behaviour (usually crashes the program).

You should allocate memory and copy the string literal there prior to reversing, for example:

char *someString = "Hi there, I'm bad at this.";
char* stringCopy = new char[strlen( someString ) + 1];
strcpy( stringCopy, someString );
strReverse( stringCopy );
delete[] stringCopy;//deallocate the copy when no longer needed
sharptooth
Personally, I'd use strdup, but I'm oldschool.
Paul Tomblin
strdup() is nice, but it requires using free() and using free(0 is not considered good C++, but rather oldschool C. Otherwise I'd also prefer strdup().
sharptooth
So what's the difference? Is it that i'm giving it its value on declaration? If I had left it as char *someString; and then passed user input to it, I could perform the operation?
Mark
The key is how the memory is allocated. If it is heap- or stack-allocated, it is allright. In origonal code it is allocated statically when the program is started and marked read-only, so writing to it is prohibited.
sharptooth
As @sharptooth said, you've declared that "somestring" is a pointer to the literal string - ie. it's pointing to some storage that is part of the program code, and therefore not modifiable. You have to put it in modifiable memory in order to reverse it.
Paul Tomblin
@sharptooth: Why did you introduce dynamically allocated strings anyway? What's wrong with a simple `char someString[] = "Hi there, I'm bad at this."`?
sbi
+1  A: 

You can't change string literals (staticly allocated). To do what you want, you need to use something like:

int main()
{
    char *str = new char[a_value];
    sprintf(str, "%s", <your string here>);
    strReverse(str);
    delete [] str;
    return 0;
}

[edit] strdup also works, also strncpy... i'm sure there's a variety of other methods :)

laura
Just asked this question to the person above and then read your answer :) Thanks.
Mark
+1  A: 

See sharptooth for explanation.

Try this instead:

#include <cstring>

void main()
{
    char someString[27];
    std::strcpy( someString, "Hi there, I'm bad at this." );
    strReverse( someString );
}

Better yet, forget about char * and use <string> instead. This is C++, not C, after all.

DevSolar
+15  A: 

If you change this, which makes someString a pointer to a read-only string literal:

char *someString = "Hi there, I'm bad at this.";

to this, which makes someString a modifiable array of char, initialized from a string literal:

char someString[] = "Hi there, I'm bad at this.";

You should have better results.

While the type of someString in the original code (char*) allows modification to the chars that it points to, because it was actually pointing at a string literal (which are not permitted to be modified) attempting to do any modification through the pointer resulted in what is technically known as undefined behaviour, which in your case was a memory access violation.

Charles Bailey
+2  A: 

The line

char *someString = "Hi there, I'm bad at this.";

makes someString point to a string literal, which cannot be modified. Instead of using a raw pointer, use a character array:

char someString[] = "Hi there, I'm bad at this.";
Bojan Resnik
A: 

When using more strict compiler settings, this code shouldn't even compile:

char* str = "Constant string";

because it should be constant:

const char* str = "Now correct";
const char str[] = "Also correct";

This allows you to catch these errors faster. Or you can just use a character array:

char str[] = "You can write to me, but don't try to write something longer!";

To be perfectly safe, just use std::string. You're using C++ after all, and raw string manipulation is extremely error-prone.

phjr
Actually it has to compile. For compatibility, there's a conversion from `const char[]` to `char*`. It's deprecated and you shouldn't sue it, but it's (still) part of the standard.
sbi
No, `char* str = "Constant string"` **must** compile. 4.2/2 says that a string literal that is not a wide string literal can be coverted to an rvalue of tpy "pointer to `char`". A conforming compiler cannot reject this initialization.
Charles Bailey
Thanks, I made my post more accurate - it's about compiler settings (which you can set to be more strict than the standard is).
phjr
If you say "You're using C++ after all", I don't think that you can really say "shouldn't even compile" when referring to strictly conforming code. You could say that you can _ask_ a compiler to reject this otherwise conforming code, but I saying that it *should* reject it is too strong (IMHO).
Charles Bailey
there's no such thing as "more strict than the standard is", since the strictness is determined by how closely it enforces the standard.
jalf
+11  A: 

If this isn't homework, the C++ tag demands you do this by using the C++ standard library:

std::string s("This is easier.");
std::reverse(s.begin(), s.end());

Oh, and it's int main(), always int main(), dammit!

sbi