0

I am new to cpp and have a question regarding arrays. The code I have below should create a reversed version of str and have it be stored in newStr. However, newStr always comes up empty. Can someone explain to me why this is happening even though I am assigning a value from str into it?

void reverse (char* str) {
    char* newStr = (char*)malloc(sizeof(str));

    for (int i=0;i<sizeof(str)/sizeof(char);i++) {
       int index = sizeof(str)/sizeof(char)-1-i;
       newStr [i] = str [index];
    }
}

PS: I know that it is much more efficient to reverse an array by moving the pointer or by using the std::reverse function but I am interested in why the above code does not work.

Code-Apprentice
  • 81,660
  • 23
  • 145
  • 268
user1330217
  • 243
  • 2
  • 4
  • 14
  • 1
    Many dupes, but `sizeof(char*)` isn't the size of the array. You should also use `std::string`, or, if you have no choice, at least `new/new[]` and `delete/delete[]` over `malloc` and `free`. – chris Sep 24 '12 at 00:42
  • 2
    Don't use malloc in C++, use new. – Borgleader Sep 24 '12 at 00:43
  • Nothing to do with `unique_ptr` – Aesthete Sep 24 '12 at 00:49
  • @Aesthete yes, he's allocating memory in a function and hoping the caller knows to deallocate the returned pointer. You need `unique_ptr` to fix that situation. – Seth Carnegie Sep 24 '12 at 00:53
  • 2
    You don't need `unique_ptr` to fix that, you need a better design and a good working knowledge of memory management. Don't throw abstractions at people before they understand what they're abstracting away. – Aesthete Sep 24 '12 at 01:00
  • @Aesthete just like you don't _need_ `unique_ptr`, you don't _need_ better design or better knowledge of memory management depending on what you are trying to do. You can have one or the other or both. And I can suggest another way of doing something without tip-toeing around to make sure he already understands it. If he doesn't, he can read about it. – Seth Carnegie Sep 24 '12 at 01:21
  • @SethCarnegie For the record, `unique_ptr` is far from the only way to fix the OP's memory leak. – Code-Apprentice Sep 24 '12 at 01:46

4 Answers4

6

As above commenters pointed out sizeof(str) does not tell you the length of the string. You should use size_t len = strlen(str);

void reverse (char* str) {
   size_t len = strlen(str);
   char* newStr = (char*)malloc(len + 1);

   for (int i=0; i<len;i++) {
      int index = len-1-i;
      newStr[i] = str[index];
   }
   newStr[len] = '\0'; // Add terminator to the new string.
}

Don't forget to free any memory you malloc. I assume your function is going to return your new string?

Edit: +1 on the length to make room for the terminator.

James
  • 1,341
  • 7
  • 13
3

The sizeof operator (it is not a function!) is evaluated at compile time. You are passing it a pointer to a region of memory that you claim holds a string. However, the length of this string isn't fixed at compile time. sizeof(str)/sizeof(char) will always yield the size of a pointer on your architecture, probably 8 or 4.

What you want is to use strlen to determine the length of your string.

Alternatively, a more idiomatic way of doing this would be to use std::string (if you insist of reversing the string yourself)

std::string reverse(std::string str) {
  for (std::string::size_type i = 0, j = str.size(); i+1 < j--; ++i) {
    char const swap = str[i];
    str[i] = str[j];
    str[j] = swap;
  }
  return str;
}

Note that due to implicit conversion (see overload (5)), you can also call this function with your plain C-style char pointer.

bitmask
  • 32,434
  • 14
  • 99
  • 159
2

There are two issues here:

  1. The sizeof operator won't give you the length of the string. Rather, it gives you the size of a char* on the machine you are using. You can use strlen() instead to get the

  2. A c-string is terminated by a NULL character (which is why strlen() can return the correct length of the string). You need to make sure you are not accidentally copying the NULL character from your source string to the beginning of your destination string. Also, you need to add a NULL character at the end of your destination string or you will get some unexpected output.

Code-Apprentice
  • 81,660
  • 23
  • 145
  • 268
0
#include <bits/stdc++.h>

using namespace std;

vector<string> split_string(string);

// Complete the reverseArray function below.
vector<int> reverseArray(vector<int> a) {
    return {a.rbegin(), a.rend()};

}

int main()
{
    ofstream fout(getenv("OUTPUT_PATH"));

    int arr_count;
    cin >> arr_count;
    cin.ignore(numeric_limits<streamsize>::max(), '\n');

    string arr_temp_temp;
    getline(cin, arr_temp_temp);

    vector<string> arr_temp = split_string(arr_temp_temp);

    vector<int> arr(arr_count);

    for (int i = 0; i < arr_count; i++) {
        int arr_item = stoi(arr_temp[i]);

        arr[i] = arr_item;
    }

    vector<int> res = reverseArray(arr);

    for (int i = 0; i < res.size(); i++) {
        fout << res[i];

        if (i != res.size() - 1) {
            fout << " ";
        }
    }

    fout << "\n";

    fout.close();

    return 0;
}

vector<string> split_string(string input_string) {
    string::iterator new_end = unique(input_string.begin(), input_string.end(), [] (const char &x, const char &y) {
        return x == y and x == ' ';
    });

    input_string.erase(new_end, input_string.end());

    while (input_string[input_string.length() - 1] == ' ') {
        input_string.pop_back();
    }

    vector<string> splits;
    char delimiter = ' ';

    size_t i = 0;
    size_t pos = input_string.find(delimiter);

    while (pos != string::npos) {
        splits.push_back(input_string.substr(i, pos - i));

        i = pos + 1;
        pos = input_string.find(delimiter, i);
    }

    splits.push_back(input_string.substr(i, min(pos, input_string.length()) - i + 1));

    return splits;
}