-5

this is the code i have for deep copying a string in copy constructor so is the logic correct??I think there may be memory leak and i am trying to fix that.m.pStatus is initialised

class Monkey
{
public:

// insert your code here
Monkey();
Monkey(const Monkey& m);
Monkey& operator=(const Monkey& m);
~Monkey();
// accessors
int getX();
int getY();
char *getStatus();
void deepCopy(const Monkey& m);
// global variables (incremented each creation or destruction)
static int numStringsCreated;
static int numStringsDestroyed;

private:
        // Do not change this data
    int x;
    int y;
    char *pStatus;

};

void Monkey::deepCopy(const Monkey& m)
{
    
    if (m.pStatus)
    {
        // allocate memory for our copy
        pStatus= new char[sizeof(m.pStatus)];
 
        for (int i{ 0 }; i < sizeof(m.pStatus); ++i)
            pStatus[i] = m.pStatus[i];
    }
    else
        pStatus = nullptr;
}


    Monkey::Monkey(const Monkey& m)
    {
        deepCopy(m);
        numStringsCreated++;
    }
Alan Birtles
  • 32,622
  • 4
  • 31
  • 60
pavithron babu
  • 1
  • 2
  • 2
    `sizeof(m.pStatus)` is very likely wrong, but without seeing the definition all we can do is guess. You probably want `strlen`. Consider a [mcve]. if `pStatus` points to previously allocated memory then this is also a memory leak. – Retired Ninja Jan 15 '22 at 00:48
  • If `pStatus` is a pointer sizeof(pStatus) is wrong. It returns the number of bytes for the pointer itself which is likely 4 for 32 bit code and 8 for 64. You probably wanted to use strlen() – drescherjm Jan 15 '22 at 00:49
  • Why are you not using `std::string` ? – selbie Jan 15 '22 at 00:55
  • `new char[]` is *not* a string, it's a character array. If you're going to code in `C++`, use the facilities given to you, not the legacy `C` stuff. Otherwise, you're that strange variety known as a `C+` developer :-) – paxdiablo Jan 15 '22 at 00:56
  • @paxdiablo is that better, worse, or the same as a Three Star Programmer? – user4581301 Jan 15 '22 at 00:58
  • Before posting their first question on stackoverflow.com, everyone should take the [tour], read the [help], understand all the requirements for a [mre] and [ask] questions here. Not doing any of this results in a poor quality question almost every time. It then gets downvoted, closed, and then deleted. – Sam Varshavchik Jan 15 '22 at 01:01
  • Side note: Rather than a member function like `deepCopy`, prefer to use the copy constructor and assignment operator. The assignment operator can be written [laughably easily](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) once you have the copy constructor up and running. – user4581301 Jan 15 '22 at 01:02

1 Answers1

1

Avoid using sizeof(pStatus). Unless the variable is fixed sized array, it will evaluate to the size of a pointer, not the length of the string.

strlen and strcpy for classic C string length and copy operations.

This is closer to what you want. It ensures that it cleans up any previous allocation and implicitly handles the case where a self assignment is made (e.g. m.deepCopy(m)) by allocating the copy first before deleting the old. You could make an additional optimization to say if (m.pStatus == pStatus) and skip the copy operation entirely.

I'm working under the assumption that pStatus was initialized to null or something valid in the constructor.

void Monkey::deepCopy(const Monkey& m)
{
    char* psz = nullptr;
    if (m.pStatus)
    {
        size_t len = strlen(m.pStatus);
        psz = new char[len+1]; // +1 for null char
        strcpy(pStatus, m.pStatus);
    }

    delete [] pStatus;
    pStatus = psz;      
}

But if pStatus was a std::string you wouldn't need to do any of this allocation stuff and you likely wouldn't need to overload the assignment operator or copy constructor.

selbie
  • 100,020
  • 15
  • 103
  • 173
  • thanks for the help.It pointed me in the right direction.my code is correct except that strcpy is not not accepted by my compiler and so i used strncpy_s.Also it needed a delete statement like in your code. – pavithron babu Jan 15 '22 at 22:34
  • Does your code correctly handle `m.deepCopy(m)` ? That is, a self-copy. From what I see above, it does not handle that implicitly. – selbie Jan 16 '22 at 06:04
  • Also, if you simply had pStatus as a `std::string` type you would not need to have a copy constructor nor overloaded assignment operator. You might not even need a destructor too. The default implementations of these operations would to the right thing. – selbie Jan 16 '22 at 06:08
  • thanks i will make that addition – pavithron babu Jan 16 '22 at 17:52