-1

I construct a class named CMyString, here is it:

class CMyString {
public:
    CMyString();
    CMyString(char* pData);
    CMyString(const CMyString& str);
    ~CMyString(void);

    char* getData();
    void setData(char* pData);

    CMyString& operator=(const CMyString& str);

private:
    char* m_pData;
};

    CMyString::CMyString() {
    m_pData = new char;
}

CMyString::CMyString(char* pData) {
//    m_pData = new char;
    m_pData = pData;
}

CMyString::CMyString(const CMyString& str) {
    // 为指针分配内存
//    m_pData = new char;
    // 拷贝值
    m_pData = str.m_pData;
}

CMyString::~CMyString(void) {
//    delete m_pData;
}

CMyString& CMyString::operator=(const CMyString& str) {
    if (this == &str)
        return *this;

    delete m_pData;
    m_pData = nullptr;
    m_pData = new char[strlen(str.m_pData) + 1];
    strcpy(m_pData, str.m_pData);

    return *this;
}

char* CMyString::getData() {
    return m_pData;
}

void CMyString::setData(char *pData) {
    m_pData = pData;
}

And here is my main.cpp:

#include <iostream>
#include "CMyString.h"

using std::cout;
using std::endl;

int main() {
    char* pData = "What are you worrying about?";
    std::cout << pData << std::endl;

    cout << strlen(pData) << endl;
    char* test = new char[30];
    cout << sizeof(test) << endl;
    char* test2 = new char;
    test2 = "23";
    cout << test2 << endl;

    strcpy(test, pData);

    cout << endl << test << endl << endl;

    CMyString str(pData);
    std::cout << str.getData() << std::endl;

    CMyString str2, str3;
    str3 = str2 = str;

    std::cout << str3.getData() << endl;

    char* pData2 = "Data has been changed.";
    str3.setData(pData2);

    cout << str.getData() << endl;
    cout << str2.getData() << endl;
    cout << str3.getData() << endl;

    return 0;
}

then I'm confused by the

char* pData = new char;
char* pData2 = new char[30];

Am I right in the class implementation? How can I tell the two different pointers? Do I write the constructor and de-constructor function and the operator= correctly? if not, how to write them?

Poodar
  • 145
  • 1
  • 2
  • 7
  • Two things: You don't want to mix `new char` and `new char[n]`. They require slightly different deleting (`delete` and `delete[]`) and it's next to impossible to figure out from the pointer which one to use. Next, `char * str= "a string";` is bad form in pre C++11 code and illegal afterwards. "a string" is a string literal and may be stored in non-writable storage. It should be a `const char *` to prevent accidental, program-crashing writes int that which cannot be written. – user4581301 Apr 02 '17 at 08:22
  • Your copy constructor results in both the source and copy pointing at the same string. This is bad once you fully implement the destructor because they will both try to destroy the same string and other badness. You will need to allocate a new buffer to hold a copy of the source's string and then copy into that buffer. – user4581301 Apr 02 '17 at 08:27
  • Helpful reading: http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom – user4581301 Apr 02 '17 at 08:36
  • 3
    Welcome to the 21st century. Nobody needs a class like `CMyString` in C++ anymore. Throw it away and use `std::string`. – Christian Hackl Apr 02 '17 at 09:09

2 Answers2

0
char* pData = new char;  -> Allocates one piece / slot of memory
char* pData2 = new char[30]; -> Allocates 30 continuous pieces / slots of memory

First one usesdelete pData, second one delete [] pData

And addition to this, you are using char *pData = "STRING", I wouldn't recommend you using that since you are setting a pointer to that STRING and not copying it to a certain location.

When it comes to the operator= it looks good as I can see if you pass the correct data..

EDIT: Seen other comments saying for the copy constructor where I didn't throw look at.

Copy Constructor it constructs / makes an instance same as the constructor so if you are dynamically allocating memory in the constructor and in the class then you should do that in the copy constructor too..

// Ofcourse #include<cstring>
class CMyString {
public:
CMyString();
CMyString(char* pData);
CMyString(const CMyString& str);
~CMyString(void);

char* getData();
void setData(char* pData);

CMyString& operator=(const CMyString& str);

private:
    char* m_pData;
};

    CMyString::CMyString() {
    m_pData = nullptr; // so it doesn't hang
}

CMyString::CMyString(char* pData) {
    m_pData = new char[strlen(pData) + 1]; // allocates slots of memory with length of the string pData
    strcpy(m_pData, pData)
}

CMyString::CMyString(const CMyString& str) {
    m_pData = new char[strlen(str.m_pData) + 1];
    strcpy(m_pData, str.m_pData);
    // Here you are constructing an instance so you should allocate memory for the dynamic string
}

CMyString::~CMyString(void) {
   delete [] m_pData; // Since we allocated few slots
}

CMyString& CMyString::operator=(const CMyString& str) {
    if (this == &str)
        return *this;

    delete [] m_pData; // Few slots of memory should be released
    // m_pData = nullptr; No need for this since you are allocating after
    m_pData = new char[strlen(str.m_pData) + 1];
    strcpy(m_pData, str.m_pData);

    return *this;
}

char* CMyString::getData() {
    return m_pData;
}

void CMyString::setData(char *pData) {
     // m_pData = pData; Illegal don't get the address of a string make a copy of it (Don't point to a string you want a copy)
     // If it isn't set. If it is, you will need to delete it
     m_pData = new char[strlen(pData) + 1];
     strcpy(m_pData, pData);
}

Possible to have some typos so hit me up if anything needs to be edited or added..

NOTE: test = "String" is illegal always copy the content if it is your intention ( strcpy ). Look at other comments if people leave because I'm limited to the current knowledge (still newbie). However, I will try to help..

Important: When allocating try to understand the concept behind it and what you want to do and what you have done. Don't forget the delete / delete [] :) Good luck !!

0

Well, you have a ton of bugs. Just not in operator=. There you only have a pointless m_pData = nullptr;.

new char allocates a single char and new char[30] allocates an array of 30 chars. The pointer is the same and you have to know which of the two it is. Every new must be balanced by a delete and every new[] balanced by a delete[]. So best to never mix the two. Use new char[1].

Some notes for the rest of your code:

1) m_pData = str.m_pData; copies the address of the array so now 2 classes use the same array and the old array is never freed.

2) The commented out delete in the destructor is needed to prevent memory leacks but should be delete[]. But 1 makes that impossible.

3) test2 = "23"; uses incompatible pointer types. C strings are const and your compiler should have warned you about this.

4) test2 = "23"; copies the addsress of the array overwriting the old value. The old array is a memory leak.

5) CMyString str(pData); causes the address of pData to be copied into your class. The delete[] in 2 would later try to free it but it was never allocated by new[]. The constructors and setData should realy copy the string like you do in operator=.

6) Have a look at std::move, std::uniq_ptr, std::shared_ptr.

Goswin von Brederlow
  • 11,875
  • 2
  • 24
  • 42