The following code is an attempt to implement the Rule of 3, but Im getting a few errors

Question

I'm getting 8 errors in this program. Can anyone help?

#define _CRT_SECURE_NO_WARNINGS

#include <iostream>
#include <string>
#include <cstring>

using namespace std;

class Name {
   const char* m_value;

   void allocateAndCopy(const char* value) {
      delete[] m_value;
      m_value = new char[strlen(value) + 1];
      strcpy(m_value, value);
   }

public:
   Name() :m_value(nullptr) {
   };

   Name(const char* value) :m_value(nullptr) {
      allocateAndCopy(value);
   }

   Name(Name& N) {
      *this = N;
   }

   Name operator=(const Name& N) {
      allocateAndCopy(N.m_value);
      return this;
   }

   ~Name() {
      delete[] m_value;
   }

   void display(ostream& os)const {
      os << m_value;
   }

   void read(istream& is) {
      char val[1000];
      getline(is, val);
      allocateAndCopy(val.c_str());
   }
};

ostream& operator<<(ostream& os, Name& N) {
   N.display(os);
   return os;
}

istream& operator<<(istream& is, Name& N) {
   return N.read(is);
}

The errors I'm getting are:

Severity    Code    Description Project File    Line    Suppression State
Error (active)  E0167   argument of type "const char *" is incompatible with parameter of type "char *"
and more.

Answer 1

There are many problems with your code:

• avoid using namespace std;

• m_value is a const char*, ie a pointer to read-only memory, but you are trying to modify the data it is pointing at (via strcpy()), so it needs to be a char* instead, ie a pointer to writable memory.

• allocateAndCopy() doesn't account for value being nullptr. Calling strlen() and strcpy() with a nullptr as input is undefined behavior.

• the copy constructor is not initializing m_value before calling operator=, thus making allocateAndCopy() call delete[] on an invalid pointer, which is also undefined behavior.

• operator= is declared to return a new Name object by value, but this is a Name* pointer. You need to return *this by Name& reference instead. You are also not accounting for the possibility of self-assignment (ie, assigning a Name object to itself).

• read() is calling std::getline() which requires a std::string not a char[]. And a char[] does not have a c_str() method. And you are not checking if getline() is even successful before passing val to allocateAndCopy().

• operator<< should take in a Name object by const reference.

• your stream extraction operator is named operator<< (the name of the stream insertion operator). It needs to be named operator>> instead. Also, it needs to return is by reference, to allow the caller to chain multiple >> reads together. It is trying to return the return value of read(), which is void.

With that said, try something more like this instead:

#define _CRT_SECURE_NO_WARNINGS

#include <iostream>
//#include <string>
#include <cstring>

class Name {
   char* m_value = nullptr;

   void allocateAndCopy(const char* value) {
      delete[] m_value;
      m_value = nullptr;
      if (value) {
          m_value = new char[std::strlen(value) + 1];
          std::strcpy(m_value, value);
      }
   }

public:
   Name() = default;

   Name(const char* value) {
      allocateAndCopy(value);
   }

   Name(const Name& N) {
      allocateAndCopy(N.m_value);
   }

   Name& operator=(const Name& N) {
      if (&N != this) {
          allocateAndCopy(N.m_value);
      }
      return *this;
   }

   ~Name() {
      delete[] m_value;
   }

   void display(std::ostream& os) const {
      os << m_value;
   }

   void read(std::istream& is) {
      char val[1000] = {};
      if (is.getline(val, 1000))
          allocateAndCopy(val);

      /* alternatively: 

      std::string val;
      if (std::getline(is, val))
          allocateAndCopy(val.c_str());
      */
   }
};

std::ostream& operator<<(std::ostream& os, const Name& N) {
   N.display(os);
   return os;
}

std::istream& operator>>(std::istream& is, Name& N) {
   N.read(is);
   return is;
}

Answer 2

There are at least four fundamental bugs/problems with the shown code.

 strcpy(m_value, value);

m_value is a const char *, a pointer to constant characters. "constant" means that you can't strcpy() over them, of course.

Instead of immediately assigning the result of new [] to m_value, and then strcpy() into it, the result of new [] should be stored in a temporary char *, then strcpy()ed; and then the temporary char * can finally be placed into the m_value.

Name(Name& N) {

A proper copy constructor should take a const reference as a parameter, this should be Name(const Name &N).

Name(Name& N) {
  *this = N;
}

This constructor fails to initialize the m_value class member. The constructor then calls the overloaded operator=. It will, eventually, attempt to delete m_value, which was never initialized. This results in undefined behavior, and a likely crash. You will need to fix this bug as well.

 Name operator=(const Name& N) {

An overloaded operator= is expected to return a reference to this, not a brand new object. This should return a Name &, and actually return *this.