3

Is it sensible or silly to pass new'd objects in a constructor initialiser?

For example:

class Mallard
{
public:
  Mallard()
   : s(new SomeLargeObject(5)) {}
private:
  SomeLargeObject * s;
};
patchwork
  • 1,161
  • 3
  • 8
  • 23
  • 1
    It's not sensible to use `new`/`delete` unless you have a very good reason to do so. – olevegard Feb 11 '14 at 22:20
  • Let's suppose I do have a very good reason for using new (my example is not that great, but let's suppose). Is it then sensible to put it in a constructor? – patchwork Feb 11 '14 at 22:22
  • 2
    @patchwork Sure, but only if you have one `new`, add a second dynamic allocation in there and now there's a chance you'll leak. Even with a single dynamically allocated member it'd be better to use a `unique_ptr` or `shared_ptr` to manage that. – Praetorian Feb 11 '14 at 22:24
  • 3
    Why not? Just make sure the destructor tidies up – Ed Heal Feb 11 '14 at 22:25
  • I wouldnt put it in the initializer list though, unless you define your pointer as `T * const`. In this case however you can actually also go with a regular `T`. – Sebastian Hoffmann Feb 11 '14 at 22:28
  • @Praetorian, why would there be a chance of leaking only on the second allocation? (don't you need only one call to new to give you a chance of a leak? ) – patchwork Feb 11 '14 at 22:29
  • @patchwork He means this: `p = new Foo(); p = new Foo();`. One object leaked due to lost reference. – Sebastian Hoffmann Feb 11 '14 at 22:30
  • @Paranaix, why? (... would you not put it in the initializer list? what's the risk? what is the benefit of having pointer to a const?) – patchwork Feb 11 '14 at 22:30
  • 2
    @patchwork I'm assuming you've added a corresponding `delete` in the destructor to prevent an obvious memory leak. However, if you initialize a second member, say `Mallard():s(new Foo()), s2(new Bar()) {}` and the second allocation fails, the destructor will not be called, thus leaking `s`. – Praetorian Feb 11 '14 at 22:31
  • @patchwork There are no risks. I would favor putting it in the constructor simply for readability and also because usually you have to do a bit more if you already have to allocate something manually on the heap. – Sebastian Hoffmann Feb 11 '14 at 22:31
  • @Praetorian - "if you initialise a second member, and the second allocations fails, the destructor will not be called" - does you mean to imply that the destructor *will* be called if the *first* allocation fails? sorry, I'm a little confused now. – patchwork Feb 11 '14 at 22:33
  • 1
    @patchwork No, of course not. What's special about the first allocation compared to the second? Read [this](https://stackoverflow.com/a/10212864/241631). With a single dynamically allocated member, if that allocation fails it doesn't matter whether the destructor is called because you didn't allocate any memory and so there's no need to `delete` it. The point I'm trying to make is that managing raw pointers along with other data members is error prone, use a smart pointer. Note that all this applies to whatever kind of resource you manage (files, sockets etc.), not just memory. – Praetorian Feb 11 '14 at 22:37
  • 1
    As a general answer: If it is wise/necessary, it is ok as long as you tidy up (as mentioned). My contribution to this comment section: Look at std::shared_ptr (not only use it, but note that it does exactly that, since you pass it new'd pointers only on construction). – Richard Vock Feb 11 '14 at 22:47
  • 4
    @patchwork: if a constructor throws an exception, the corresponding destructor is NOT called. So, if you have something like `Mallard():s(new Foo()), s2(new Bar()) {}` and `s(new Foo())` succeeds and then `s2(new Bar())` throws, `~Mallard()` is not called and `s` is leaked **UNLESS** `s` is declared as a smart pointer (like `auto_ptr s` or `unique_ptr s`), in which case the smart pointer's destructor WILL be called (when a constructor throws, members already constructed get destructed) and the memory will be freed as expected. – Remy Lebeau Feb 11 '14 at 23:01
  • @Remy why if i don't want to use smart pointers,can't i catch exception and therefore handle memory leak – PapaDiHatti May 28 '16 at 01:46
  • @Kapil: doing so would make the code sloppy, when you can just let the compiler do all the work for you. But, if you must, an exception thrown in a constructor's initialization list can be caught using a [function-try-block](http://en.cppreference.com/w/cpp/language/function-try-block), eg: `Mallard() try : s(new Foo()), s2(new Bar()) { /*constructor body*/ } catch(const std::exception& e) { /*exception handler*/ }`. Just know that you cannot swallow a caught exception with this technique. When the `function-try-block` is finished, a caught exception will be automatically re-thrown. – Remy Lebeau May 28 '16 at 02:28
  • @Remy And this is good that exception is rethrown otherwise there is no way to get any failure string or failure code from constructor since they cannot return – PapaDiHatti May 28 '16 at 08:52

3 Answers3

5

There is nothing mechanically broken in your code sample if the destructor, which you forgot to publish, does the symmetric deletion.

Though conventionally (safer and less boilerplate) you'd want something like

#include <memory>

class Mallard {
    std::unique_ptr<SomeLargeObject> that_;
public:
     Mullard(): that_(new SomeLargeObject)
     {}
     // this way you don't need to code the destructor
};

Note: As people highlighted in comments (@Praetorian,@RemyLebeau,@AdrianMcCarthy), if your had been initializing more than one member this way it wouldn't have been subject to a memory leak as when an exception is thrown from the object constructor the destructor for this object is not called. The above suggested syntax with std::unique_ptr is not subject to this problem (this is why it is safe).

Community
  • 1
  • 1
bobah
  • 18,364
  • 2
  • 37
  • 70
  • 2
    It would be good to point out that if the constructor had to dynamically allocate two or more objects, then there would be a chance of a leak, and using a smart pointer then becomes essential. – Adrian McCarthy Feb 11 '14 at 23:36
  • @MooingDuck - I don't think I misunderstand anything. You can check the stdout of this one and run it under valgrind: http://ideone.com/cDiYVN – bobah Feb 12 '14 at 00:55
  • 2
    @MooingDuck - I'll just leave it here: http://stackoverflow.com/a/1589987/267482 for two anonymous downvoters – bobah Feb 12 '14 at 07:25
  • 1
    @bobah: Well don't I look foolish. I hadn't realized that each mem-initializer is considered a full-expressions that is therefore ordered, avoiding the potential memory leaks. Things learned +1. – Mooing Duck Feb 12 '14 at 17:09
1

If I may throw my 2 cents:

Cent #1: Generally speaking, it would be better to make s a member of type SomeLargeObject and not concern oneself with dynamic memory allocation;

Cent #2: Sometime it is useful to allocate things in a constructor like that, and, provided that you are careful, there is nothing wrong with that. In particular, this is useful for Bridge Pattern like this:

// Abstract base of all beaks
class Beak
{
  public:
};

// Abstract base of all birds
class Bird
{
  protected: 
    const Beak *beak;

    ~Bird()
    {
        delete beak;
    }
};

class MallardBeak : public Beak
{
};

class Mallard : public Bird
{
  public:
    Mallard() : beak(new MallardBeak) { }
};

class PigeonBeak : public Beak
{
};

class Pigeon : public Bird
{
  public:
    Pigeon() : beak(new PigeonBeak) { }
};
Michael
  • 5,775
  • 2
  • 34
  • 53
0

Because of C++ not having a Garbage Collector (like Java, for example), you must be generally very careful on using dynamically (on heap) created objects, in general. If you miss correctly deallocating the memory that was dynamically allocated at some point, you will end up having a memory leak.

Also, on the constructor case, what if an error occurs on object creation (an exception is raised, for example) but the dynamically object is created before the exception call? Just some food for thought, you know.

Again, be very careful, and check on some techiques that protect you from such mistakes and unwanted situations like this, for example Smart Pointers, that use the RAII idiom. (Boost Libraries collection has some great implementations, giving you a variety of smart pointers you can use depending on your needs).

Nick Louloudakis
  • 5,856
  • 4
  • 41
  • 54