1

In code I use I found a class declaration in a header file that contains a pointer to const as a member that is defined with 'new' right there in the header file.

A matching 'delete' is in the destructor (also defined right there in the header).

Is this usage ok? Does it create a memory leak? The destructor is called only when an object of that class is destroyed, either by a delete (when created with new) or by going out of scope (when created on the stack). But the 'new' is not in the constructor, but part of the class declaration. Isn't this executed earlier? Maybe even whenever the header is parsed? Is it guaranteed to me matched by the delete in the destructor?

class Foo {
public:
  explicit Foo(){}
  ~Foo() {
    delete this->bar;
  }

private:
  const Baz* bar = new Baz();
};
Chaos_99
  • 2,284
  • 2
  • 25
  • 29
  • Why not `new` in constructor `Foo::Foo() { bar = new Baz(); }`? – i486 Feb 20 '19 at 12:27
  • 1
    @i486 why would it be? The real question is, why no `std::unique_ptr`. – Quentin Feb 20 '19 at 12:28
  • @Quentin Explain "the real question" because I don't see it in that way. – i486 Feb 20 '19 at 12:33
  • @i486 [yup](https://wandbox.org/permlink/wfSnMhC1vAk2MBya) (although the MCVE wasn't quite there, OP!) – Quentin Feb 20 '19 at 12:33
  • 3
    the pointer itself is not constant, its a non-const pointer pointing to a `const Baz` – 463035818_is_not_an_ai Feb 20 '19 at 12:33
  • Pedantic: you do not have a 'const member pointer'. Instead, you have a 'member pointer to const Baz'. – Krzysiek Karbowiak Feb 20 '19 at 12:33
  • @KrzysiekKarbowiak its not really pedantery, but `const Baz*` and `Baz * const` are very different – 463035818_is_not_an_ai Feb 20 '19 at 12:34
  • Intializing non static member at declaration time is just systactic sugar: the initialization will occur at object creation, no problem here. But you as the destructor is not trivial, the rule of 5 require special processing for copy/move. You'd better use a `std::unique_ptr` and let it manage the destruction. – Serge Ballesta Feb 20 '19 at 12:35
  • 1
    @i486 it was a bit tongue-in-cheek. You're advocating symmetry, I'm advocating following the rule of zero and using `std::unique_ptr` instead of a raw owning pointer :) – Quentin Feb 20 '19 at 12:35
  • For clarification: That's library header code, not my code. That's why I'm asking for explanation only, not how to fix. But thanks for the suggestions anyway. – Chaos_99 Feb 20 '19 at 12:41
  • I fixed the wording around what is const here to match the code. thanks for the advise. – Chaos_99 Feb 20 '19 at 12:53

1 Answers1

7

Does it create a memory leak?

Potentially, yes. If you assign to an instance of Foo, the memory owned by the previous pointer value is leaked.

Isn't this executed earlier? Maybe even whenever the header is parsed?

No. Members are initialized by a constructor. The default member initialiser in the declaration of the member is used if the constructor doesn't specify a member initialiser explicitly (such as is the case for the constructor you declared).

Is this usage ok? Is it guaranteed to me matched by the delete in the destructor?

No. Besides the leak, if you make a copy of the object, there will be two deletes matching only one new. The program would have undefined behaviour.

The simplest fix for both the leak, and UB is to use std::unique_ptr instead of a bare pointer. (Or possibly std::shared_ptr in case you want the class to be copyable, and want the copies to share the ownership of the same Baz object).

There is no problem with the default member initialiser though. The problems mentioned above are with lack of class invariants that are necessary when dealing with resource acquisition.

eerorika
  • 232,697
  • 12
  • 197
  • 326
  • The constructor->destructor in the question was indeed a typo. Fixed that. Thanks. – Chaos_99 Feb 20 '19 at 12:39
  • @Chaos_99 I removed obsolete mention from the anwer. – eerorika Feb 20 '19 at 12:40
  • 2
    Note that using std::unique_ptr would mean that your object becomes non-copyable(L) because std::unique_ptr can't be copied except from an r-val. This is fine if it is what you want. You need to decide for yourself what copy semantics Foo should have, and preferably define them up front, not rely on the inherited side-effects. – Gem Taylor Feb 20 '19 at 12:40
  • 1
    dont agree 100% to "does not leak". When you make a copy as in `Foo f; Foo g; f = g;` you only get a double free corruption when both go out of scope, but before that there is a leak – 463035818_is_not_an_ai Feb 20 '19 at 12:43
  • @user463035818 ah. I had assumed that pointer was const, as OP stated, and thus assignment wouldn't have been possible. But it isn't actually const, so leak is possible in that case. – eerorika Feb 20 '19 at 12:45
  • i mean its a nitpick, its definitely wrong to naively copy those objects, for what reason is kinda secondary – 463035818_is_not_an_ai Feb 20 '19 at 12:46
  • FYI: The object is indeed never copied in my code. So I'm probably ok. I'll submit a change adding explicitly deleted copy constructor/operator and the use of std::unique_ptr instead of normal pointer to the library maintainer of that header file. Thanks for the help. – Chaos_99 Feb 20 '19 at 13:06
  • I believe it would be helpful to link to the [rule of three/five](https://stackoverflow.com/a/4172724/4341534) in your answer. – patatahooligan Feb 20 '19 at 13:40