c++ pointer problem. not sure what im doing wrong

Question

I have a question. I have the following struct:

typedef struct{
    int vin;
    char* make;
    char* model;
    int year;
    double fee;
}car;

Then I have the following method that asks the user for the make of a car and returns it as a char pointer

char* askMake(){
    char* tempMake = NULL;
    cout << "Enter Make:" << endl;
    cin >> tempMake;
    return tempMake;
}

Then I have a temp car struct

car tempCar;

And I am trying to assign a value to it this way

tempCar.make = askMake();

It compiles fine, but I get a segmentation fault at runtime.

Any ideas what Im doing wrong?

Answer 1

You have to allocate memory for tempMake.

Try this:

char* askMake(){
    char* tempMake = new char[1024]; //Arbitrary size
    cout << "Enter Make:" << endl;
    cin >> tempMake;
    return tempMake;
}

Don't forget to free with delete[] the memory that you allocated.

If you don't want memory leaks, you can avoid this using smart pointers like boost::shared_ptr or boost::scoped_ptr or similar. You can see more about this here.

Answer 2

You really want to use std::string here instead of char*. The problem is that you are trying to read user input into memory (tempMake) that has not yet been allocated.

std::string askMake(){
    std::string tempMake;
    cout << "Enter Make:" << endl;
    cin >> tempMake;
    return tempMake;
}

You will also probably want to use std::string instead of char* in your 'car' struct as well.

Answer 3

You haven't allocated any memory for tempMake to point at. When you read in the data, it's reading it into whatever random location tempMake happens to point at.

Get rid of the pointers and use std::string instead to make life a lot simpler.

Answer 4

You're getting a segfault because you're writing to a null pointer. You should create a new memory space for cin to write to, then copy it when it returns. std::string can do this for you:

std::string askMake() {
    std::string temp;
    cout << "Enter Make:" << endl;
    cin >> temp;
    return temp;
}

Answer 5

Others have told you what needs to be done to fix the immediate problem: either allocate space for tempMake using new or malloc, or else use a std:string.

You probably don't want to return a pointer to a struct's member from a function. While you can make correct code while doing so, and there are also very good reasons to do so, this might not be one of those instances. The problem has to do with ownership. If you expose the variable by pointer, then the end user is free to pass that guy around into other functions that may eventually free it before you want them to, or change it in some other way. Additionally, what happens when you decide to free that memory yourself? What if the guy on your team who doesn't know your code was using that pointer value after you deleted it? What if nobody frees it and you use this struct over and over? This is a memory leak.

The best model is to hide this functionality is to no allow direct access to your class members, and don't return a pointer from a function unless absolutely necessary. In C++, I think the most elegant solution would be to return a std::string. In straight C, instead pass a char** (let's call it x) into the function, and do this:

int askMake(char** x)
{
    char tempMake[100];//or some value you know to be large enough
    cout << "Enter Make:" << endl;
    cin >> tempMake;//i would use cin.get() so you know the length of the string.
    //so let's pretend we have that length in a variable called stringLen.

    *x = new char[stringLen];
    for(int i = 0; x && i < stringLen; i++)
    {
        (*x)[i] = tempMake[i];
    }

    if(x)
       return 0;
    else
       return 1;
}

Answer 6

As others have said, you're giving yourself extra work by using char* instead of std::string. If you switch over to std::string it would look like this:

#include <string>
struct car
{
  int vin;
  std::string make;
  std::string model;
  int year; 
  double fee; 
}; 

std::string askMake()
{
  std::string make;
  cout << "Enter Make:" << endl;
  cin >> make;
  return make;
}

int main()
{
  car tempCar;
  tempCar.make = askMake();
}