Deleting a pointer in destructor C++

Question

I am using a class from a library. Let it be A, and it has a character pointer "token"

My code:

void someFunction()
{
    A a;
    cout<<a.token;
    anotherFunction(a);
    cout<<a.token;  //  line 4: now token became invalid [1]
}

void anotherFunction(A copyOfA);
{
   //  doing something
}  //  on exit destructor of copyofA will be called

[1] Why did it become invalid: class A is as follows:

class A
{
    char *token;
    public:
    A()
    {
        token = GetRandomToken();   // GetRandomToken will return a 'new Char' array
    }
    ~A()
    {
        if(token != NULL)
        {
            delete[] token;    // it is A's responsibility to delete the memory it created
            token = NULL;
        }
    }
};

in anotherFunction when the destructor of copyOfA is called token got deleted. So at line 4, token is invalid because both a.token and copyOfA.token both pointing to same address.

What is the solution, in following case:

case 1: class A is in a given library: So I can't modify it.

case 2: if I can modify class A: What will be the good way to handle this?

I know, if anotherFunction is called by passing reference, I won't have hit this problem. But what if I have to keep a copy of the object at some point?

Check sample code here: https://ideone.com/yZa4k4

`class A` does not follow the [rule of 3/5](http://en.cppreference.com/w/cpp/language/rule_of_three). You cannot copy it safely — UnholySheep, May 17 '18 at 08:58
Possible duplicate of [What is The Rule of Three?](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) — UnholySheep, May 17 '18 at 08:59
You can use std:unique_ptr to hold only one copy of class A or std::shadred_ptr if class A is a sharable resource. — Unick, May 17 '18 at 09:00
@UnholySheep: No, I am given this class (from library). I want to know if there is some work around, so I can use this class? — Vivek MVK, May 17 '18 at 09:02
The only workaround is to never create a copy. You can only have a single instance and pass around references or pointers to it. — UnholySheep, May 17 '18 at 09:04
You could wrap class `A` in another class `B` with proper implementation and never use `A` directly. Question is: what do you intend a copy of `A` to do? Copy the content pointed out by `token`? Or both instance should share `token`? — Holt, May 17 '18 at 09:08
Throw the library away because it is god awful. If you can modify it, just give it proper copy semantics — Passer By, May 17 '18 at 09:15
To echo @PasserBy, throw the library away, because if it's authors get this wrong, then who knows what else they have gotten wrong? — Caleth, May 17 '18 at 10:17
@PasserBy, come on, most of the libs I was working with (mostly some hardware API) are really badly designed, but if I threw away everything that is not well written I would have to write all the stuff on my own ... — pptaszni, May 17 '18 at 11:47

score 3 · Answer 1 · answered May 17 '18 at 11:39

If you cannot modify class A, then you should avoid copying it. I think that the most safe way to do it is to allocate object of class A dynamically:

void anotherFunction(std::shared_ptr<A> aPtr)
{
    // please also note that in your case token is PRIVATE
    std::cout << aPtr->token << std::endl;
}

std::shared_ptr<A> aPtr(new A);
std::cout << aPtr->token << std::endl;
anotherFunction(aPtr);

Or if you insist on stack allocation, you should change anotherFunction signature to:

void anotherFunction(const A& a)
{
    std::cout << a.token << std::endl;
}

to pass your argument by const reference (avoid copy-constructor).

Now, if you CAN modify your class A, you should apply the rule of three/five/zero, because you have non-trivial destructor. The lazy way to do this would be to just declare other constructors as deleted (then, like in your example, you cannot copy your A object, but also you have a guarantee that no one will attempt to do so):

class A
{
    public:
    // for this example purpose I made token PUBLIC, but it is a bad idea in general
    char *token;
    A()
    {
        token = GetRandomToken();   // GetRandomToken will return a 'new Char' array
    }
    ~A()
    {
        if(token != NULL)
        {
            delete[] token;    // it is A's responsibility to delete the memory it created
            token = NULL;
        }
    }
    A(const A& other) = delete;
    A(A&& other) = delete;
};

Or, if you are not lazy, you can actually think about how to copy the memory from token pointer in one object to another object - it is up to you how you would implement it. That depends on the requirements and implementation of GetRandomToken.

Reinhold · Answer 2 · 2018-05-17T14:05:50.927

-1

If your example is accurate then class A doesn't have a proper copy constructor and therefore deletes the token for both instances. Which leads to a double delete because the pointer in the first instance didn't change.

edited May 17 '18 at 14:05

answered May 17 '18 at 13:43

Reinhold

546
1
3
14

Deleting a pointer in destructor C++

2 Answers2