CODEWITHSUNDEEP
c
Mark
Mark

Reputation: 6351

Memory access violation. What's wrong with this seemingly simple program?

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);

}

Upvotes: 3

Views: 1106

Answers (7)

sbi
sbi

Reputation: 224129

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!

Upvotes: 11

CB Bailey
CB Bailey

Reputation: 792547

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.

Upvotes: 15

Paweł Hajdan
Paweł Hajdan

Reputation: 18552

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.

Upvotes: 0

Bojan Resnik
Bojan Resnik

Reputation: 7388

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.";

Upvotes: 2

DevSolar
DevSolar

Reputation: 70333

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.

Upvotes: 1

laura
laura

Reputation: 7332

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 :)

Upvotes: 1

sharptooth
sharptooth

Reputation: 170509

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

Upvotes: 2

Related Questions