A local variable is "local" after all

I have found a code that "sounds" like this:

struct StructureC {
   char * theString;
};

class ClassCPP : public StructureC {
public:
   ClassCPP();
   ClassCPP(const ClassCPP& aClass);

   ~ClassCPP();
};

ClassCPP::ClassCPP() {
   std::string myString = foo();

   theString = myString.c_str();
}

ClassCPP::~ClassCPP() {
}

ClassCPP::ClassCPP(const ClassCPP& aClass) {
   theString = aClass.theString;
}

someone failed here, badly!

Let's see.

Of course the code above is not useful, it was simplified and extracted from a real case; StructureC can not be changed at all.

std::string has the method c_str() that returns a null terminated sequence of characters (same content as std::string) and it points to an internal location of std::string, when the scope of myString (note how I use the prefix "my" for local variables) ends, any reference to its internal status is then not valid, unfortunately theString is a member of the object being constructed. The code can "work" but then you are very lucky if it does.

The correct way is to allocate memory in the constructor and then copy the character sequence:

ClassCPP::ClassCPP() {
  std::string myString = foo();

  theString = new char[myString.size()+1]; //+1 to store the null termination
  strcpy(theString, myString.c_str());
}

so, you think that's all, don't you?

Still some errors left.

Copy constructor shall be rewritten, if we leave it unchanged like this

ClassCPP::ClassCPP(const ClassCPP& aClass) {
  theString = aClass.theString;
}

then as soon we copy an object of type ClassCPP we will have two instances that are pointing to the same memory area (the one that contains the string).

So it'd be better to write it in the correct way:

ClassCPP::ClassCPP(const ClassCPP& aClass) {
  theString = new char[strlen(aClass.theString)+1];
  strcpy(theString, aClass.theString);  
}

I could have used a strdup but strdup uses malloc to allocate memory.

Having done dynamic allocation of memory the destructor can not be void, we need to release the memory allocated:

ClassCPP::~ClassCPP() {
  delete []theString;
}

still some problems left, incredible how many errors can be done in a few lines of code!

Assignment operator shall be written as well but in this case we can declare it
private and not implement it:

class ClassCPP : public StructureC {
  ...
private:
  const ClassCPP & operator=(const ClassCPP&);
};

in this way we can check if someone needs it (the original implementation didn't
have it implemented so I guess the intention was: "I don't need it") and if necessary
implement it.

The problems are not over yet, look at the following usage of that code:

StructureC *a = new ClassCPP;
delete a;

as you can see deleting an instance of ClassCPP through a pointer to its base class will not call the ClassCPP destructor. Then we need to declare the destructor of StructureC virtual but given the fact we can not change StructureC then we need avoiding someone being able to build a ClassCPP in the heap memory. This can be done declaring and not implementing the operator new in the private part of class.

class ClassCPP : public StructureC {
  ...
private:
  ...
  void * operator new(std::size_t);
  void * operator new(std::size_t, void *);
};

as you can see in order to avoid any mistake better to disable the in-place operator
new as well.