0

For example,

class Test{
private:
   int* foo;
public:
   Test(int* foo){this->foo = foo;}
}

In this case, is there any way I can delete foo in the destructor? Will I have to delete foo in the destructor or at least set it to nullptr?

Evg
  • 25,259
  • 5
  • 41
  • 83
OneMoreGamble
  • 117
  • 1
  • 1
  • 5
  • 1
    Why would you name a member and a constructor agument with the same name? – MatG Dec 04 '21 at 07:12
  • Do you _want_ to delete foo in the destructor? Where did foo come from and who created it with `new`? – jtbandes Dec 04 '21 at 07:13
  • 2
    You cant call `delete` on something that wasn't created with `new`. Simple as that. If the caller of `Test()` passes in a pointer to a `new`'ed object, then `delete` it. Otherwise don't. This is why using raw pointers is so ambiguous. Use smart pointers instead to make ownership semantics more explicit. – Remy Lebeau Dec 04 '21 at 07:13
  • 1
    If the contract of your class constructor is that it's supposed to take ownership of the pointer, then it needs to be responsible for deleting the pointer. – JohnFilleau Dec 04 '21 at 07:14
  • 3
    @MatG Because it is a quite common practice. There is no much disadvantages to it and it allows to easier determine what this argument is for. – Piotr Siupa Dec 04 '21 at 07:15
  • @NO_NAME I didn't know this is a common practice, but I'm sure it would piss off a lot of poor code reviewers. – MatG Dec 04 '21 at 07:32
  • 2
    @MatG The practice works reasonably well (as far as readability goes) if the parameter is not used in the function body, which suggests a member initializer list is used, as in `Test(int* foo) : foo(foo) {}`. It might seem odd the first time you see it, but it's easy to get used to. – JaMiT Dec 04 '21 at 08:01
  • Concerning "set it to nullptr" -- there's no point in doing that. After the destructor has run, the object doesn't exist, so its `foo` field doesn't exist. – Pete Becker Dec 04 '21 at 13:30

2 Answers2

5

Sure, you can. It is legal syntax. And depending on the rest of your program, it might do what you want, or it might be a double delete (delete the same pointer twice) which corrupts the heap and will lead to an eventual crash. (Debugging compilers might catch this.)

One of the reasons the C++ standard template library gave us shared_ptr and unique_ptr is that memory management by hand is hard. It isn't impossible; people did it (or tried to) for many, many years. But it was also a source of many runtime disasters, either the double delete (also called a premature free from the old malloc/free routines), or the opposite, a memory leak. Automatic memory management was one of Java’s selling points as a C/C++-similar language without the memory bug risk. Later C# made the same pitch to users.

I suggest using the STL and thinking about the ownership semantics of your foo. Maybe the Test class should own it; maybe it should share it; maybe it should really have a weak reference. Can't tell from a program fragment. I can only say you should review modern ideas of memory management in C++ and adopt them.

Andrew Lazarus
  • 18,205
  • 3
  • 35
  • 53
  • 4
    *"or it might be a double delete"* -- or it might be an attempt to delete a non-dynamic variable (automatic or static storage duration), which is also bad. – JaMiT Dec 04 '21 at 07:55
1

It is semantically legal to delete this pointer but I guess this isn't why you're asking. You're asking if this is a good idea.

Handling raw pointers in C++ is generally a bad practice and this problem is one example of why. To make using of the class obvious for anyone who reads it, I would use std::unique_ptr if you intend to take ownership of the pointer and either a reference or std::shared_ptr if you don't.

Smart pointer objects will track for you if the pointer was already deleted and they will delete the pointer automatically in their destructors.

If you absolutely have to have a raw pointer and you need to check if it was deleted, I'm afraid your only option is to be consistent in assigning nullptr to it right after deletion.

Piotr Siupa
  • 3,929
  • 2
  • 29
  • 65