Why initializing a second object does modify my first object?

Question

I have a class IStream2:

class IStream2 {
   private:
      char* fn;
   public:
      IStream2(char* filename);
      char* get_filename();
};


IStream2::IStream2(char *filename) {
   strcpy(fn, filename);
}
char * IStream2::get_filename() {
   return fn;
}

And here is the main code:

vector<IStream2> istreams;

char fn[] = "file1.txt";
IStream2 reader2 = IStream2(fn);
istreams.push_back(reader2);

char fn2[] = "file2.txt";
IStream2 reader3 = IStream2(fn2);
istreams.push_back(reader3);
cout << istreams[0].get_filename() << endl;

It prints file2.txt but I expected file1.txt. I know that I should use string but I would like to resolve this problem.

how have you allocated storage for fn? (so the strcpy has somewhere to copy to) — Jimmy, Jan 07 '17 at 20:19
Maybe because you are using `fn` in both cases? Change name of the second `char*` — , Jan 07 '17 at 20:21
That code is incorrect as you defined `fn` twice. Give us the real code that cause the problem. Also try to allocate `fn` member, if not you have undefined behavior. — Jean-Baptiste Yunès, Jan 07 '17 at 20:23
Sorry, I had to rewrite a simplified code for the question. I've edited the code of the problem. @Jimmy so the problem is allocation ? — zbeyens, Jan 07 '17 at 20:33
if you haven't allocated storage for fn then you have the problem that the strcpy's behaviour is undefined. This is a problem and could feasibly expain the behaviour you are experiencing if fn has been defaulted to the same location in both instances of IStream2 — Jimmy, Jan 07 '17 at 20:35
@MichaelO the allocation of fn needs to take place in the class definition. something along the lines of int len = strlen(filename); fn = new char[len+1]; strcpy(fn,filename); — Jimmy, Jan 07 '17 at 20:41
Why not use `std::string` and save yourself a lot of headaches? Allocating memory for the string may seem to work fine, but you'll want to delete it in the destructor, and you'll need extra code to follow the rule of three, etc. — Retired Ninja, Jan 07 '17 at 20:43
@Jimmy ok that's more clear. So now, it prints the good one. Thanks! — zbeyens, Jan 07 '17 at 20:52

score 2 · Accepted Answer · edited May 23 '17 at 11:46

IStream2::IStream2(char *filename) {
   strcpy(fn, filename);
}

Allocates no storage for fn. strcpy(fn, filename); invokes undefined behaviour writing into whatever storage fn points at, and after that all bets are off. The program could do anything.

The right answer is to use std::string

class IStream2 {
private:
    std::string fn;
public:
    IStream2(const char* filename); // note const. if not modifying a passed rference, 
                                    // mark it const. The compiler can make optimizations 
                                    // and can catch mistakes for you
                                    // also the function can now receive string literals
    const char* get_filename(); // another const this is because a string won't 
                                // easily give you a non const pointer
}; <-- note the added ;

IStream2::IStream2(const char *filename): fn(filename) {
}
const char * IStream2::get_filename() {
   return fn.c_str(); // get the char array from the string
}

But I suspect this is an exercise in writing C with Classes, so back into the stone ages we go. This is a LOT more work because we have to manage all of the memory ourselves. For example, We need to observe the Rule of Three. What is The Rule of Three?

Sigh.

class IStream2 {
private:
    char* fn;
public:
    IStream2(const char* filename); // note const char *
    ~IStream2(); // need destructor to clean up fn. This means we need to
                 // comply with the Rule of Three
    IStream2(const IStream2 & src); // copy constructor
    IStream2 & operator=(IStream2 src); // assignment operator
    char* get_filename(); // Note: by returning a non const pointer here we
                          // allow the caller to tamper with the contents of 
                          // fn and even delete it. This defeats the point 
                          // of declaring fn private, so avoid doing this.
};

IStream2::IStream2(const char *filename) {
   fn = new char[strlen(filename) +1]; // allocate storage. 
                                       // The +1 is to hold the string's NULL terminator
   strcpy(fn, filename);
}
// implement copy constructor
IStream2::IStream2(const IStream2 & src) {
   fn = new char[strlen(src.fn) +1];
   strcpy(fn, src.fn);
}
// implement destructor
IStream2::~IStream2()
{
    delete[] fn;
}
// implement assignment operator. Using Copy And Swap Idiom 
IStream2 & IStream2::operator=(IStream2 src)
{
    std::swap(fn, src.fn);
    return *this;
}
char * IStream2::get_filename() {
   return fn;
}


int main()
{
    vector<IStream2> istreams;

    const char* fn = "file1.txt"; // note const char *. string literals may be
                                  // read-only memory and can't be changed
    IStream2 reader2 = IStream2(fn);
    istreams.push_back(reader2);

    const char* fn2 = "file2.txt"; // and again const char *
    IStream2 reader3 = IStream2(fn2);
    istreams.push_back(reader3);
    cout << istreams[0].get_filename() << endl;
    return 0;
}

Since we are wresting with dinosaurs, I won't bother with the Rule of Five and move operations, but see how much more annoying it is to have to do this the wrong way?

Why initializing a second object does modify my first object?

1 Answers1