*new is always wrong. ALWAYS

Question

So as to explain about pointers and references in this question I wrote this code.

MyClass& MyClass::MyInstance()
{       
    static MyClass & myLocalVariable = * new MyClass(/*parameters*/);
    return myLocalVariable ;
}

one of the comments, by a really impressive high reputation SO user, simply states: *new is always wrong. ALWAYS.

It is the first time I'm told about this: Is it a famous coding standard we all should know about ? What are the reasons behind ?

In the origianl question, I also told about using a std::shared_ptr rather than a vanilla pointer — Stephane Rolland, Nov 22 '13 at 14:00
If you `new`, you need to `delete`. In this case, you can't. — crashmstr, Nov 22 '13 at 14:01
@crashmstr, of course you can delete it, it's just ugly: `delete &myLocalVariable;`. — Kijewski, Nov 22 '13 at 14:04
@Nim This is a singleton. He probably doesn't want to delete the instance. — James Kanze, Nov 22 '13 at 14:04
@Kay, I don't think you can take the address of a reference. — Jo So, Nov 22 '13 at 14:05
@JamesKanze, yes, but it should be cleaned up at *some point*(?) — Nim, Nov 22 '13 at 14:05
@Nim What do you mean by "cleaned up"? There may be some cleaning up necessary (the instances of `std::cout` etc. _are_ flushed during shut down, although they are never destructed), but such cases aren't necessarily the most common. — James Kanze, Nov 22 '13 at 14:07
@JoSo Why not? A reference is just another name for the object, and you can do anything with the reference that you could do with a variable; it just applies to the referenced object. — James Kanze, Nov 22 '13 at 14:08
In this case it's not "wrong" as such since you can still delete the object. It's merely insane. Using it to initialise an object rather than a reference, `Thing t = *new Thing;`, would be wrong, in the sense of having an unfixable memory leak. — Mike Seymour, Nov 22 '13 at 14:11
Using placement new, with a standard layout object, on an otherwise managed piece of memory, would be a possible application. It does however seem like a stretch, and not likely found in any real code. — edA-qa mort-ora-y, Nov 22 '13 at 14:24

score 9 · Accepted Answer · answered Nov 22 '13 at 14:01

9

I'm normally pragmatic, however this is too much even for me!

static MyClass & myLocalVariable = * new MyClass(/*parameters*/);

Seriously? Why not simply:

static MyClass myLocalVariable{/*parameters*/};

answered Nov 22 '13 at 14:01

Nim

33,299
2
62
101

1

That's the point... If you're doing a singleton you do not need do do this `*new` stuff. – Johan Nov 22 '13 at 14:03
What if `new` uses a very fancy allocator that you want used for this static object? – RedX Nov 22 '13 at 14:04
1

@RedX, why would you want to use a fancy allocator for a singleton? – Nim Nov 22 '13 at 14:05
Because it will result in the instance being deleted, causing potential order of destruction problems. Most of the time, in a singleton, the instance is never deleted. – James Kanze Nov 22 '13 at 14:05
@JamesKanze, that's one thing that I'm still not sure about, but so far, I've not see any cases of this (ooo destruction problems - which doesn't necessarily mean anything...) – Nim Nov 22 '13 at 14:12
@JamesKanze That's bad design IMHO. An object created should be deleted, just for the sake of future evolution and maintenance of your software. If someone ever add something to your class that will be done in the destructor, it will never be done. If you try to "unleak" your software you'll have false positives, etc... – Johan Nov 22 '13 at 14:13
@Johan: Pragmatically, we sometimes need to deal with bad design, and a leaky singleton might be a better option than re-architecting a shambling behemoth of legacy code to manage object lifetimes properly. (Of course, you should never introduce such things until you've already abandoned all hope of a clean design.) – Mike Seymour Nov 22 '13 at 14:23
@MikeSeymour I pragmaticaly have to fight with bad design on a daily basis ;) And singleton and other badly managed object lifes is one of my problem because, our program live in a C#/C++ world and when C++ dev forget to free something, C# garbage collector does not, sometimes after DLL unload. :) – Johan Nov 22 '13 at 14:33
@Johan If you delete the object, you open yourself to order of destructor problems. Sometimes (but they are rare), you can't avoid it, and I have singletons which are destructed (and their documentation makes it clear that you _cannot_ use them in a destructor of anything which might eventually have static lifetime). But in my experience, this isn't the case of most singletons, and it would be bad engineering to introduce an artificial restriction where it serves no useful purpose. – James Kanze Nov 22 '13 at 14:58
@Nim Maybe you haven't seen it because the objects you used in the destructors of your static objects weren't ever destructed. The standard, for example, states clearly that the none of the standard iostream objects will ever be destructed. – James Kanze Nov 22 '13 at 15:00
@JamesKanze, that is interesting, I had no idea about the `std::cout` etc. learn something new every day... :) – Nim Nov 22 '13 at 15:09

score 3 · Answer 2 · answered Nov 22 '13 at 14:00

3

The most obvious reason is that if you don't keep a copy of the pointer which new returned, you're not likely to ever call delete on it.

On a more human level, it will make people reading your code think less of you, and that's never a good thing either.

answered Nov 22 '13 at 14:00

Will Dean

39,055
11
90
118

8

It will make people reading your code *want to murder you*. FTFY – Thomas Nov 22 '13 at 14:01
In this case would you be able to get the pointer from the object reference? (Not that this would be a good way to go about things...) – fjc Nov 22 '13 at 14:02
1

@Frank - yes, because a C++ reference is just a disguised pointer, so you can get back to the pointer easily enough. It's still an awful-looking thing to do. – Will Dean Nov 22 '13 at 14:05

score 0 · Answer 3 · answered Nov 22 '13 at 14:00

I believe the stated user meant that allocating a static object using new is dangerous as the memory will most probably be leaked. Even more importantly your variable is not a pointer but a reference so the chance that you never free the memory returned by new is even greater(how often do you delete the address of a reference?).

score 0 · Answer 4 · answered Nov 22 '13 at 14:12

THE problem is more than just useless allocations.

If nobody calls delete on it, it won't be deleted. Sure, the memory will be released when the program ends but its destructor doesn't get called.

Compare the output of using Get() and Get2() in the code below:

    #include <iostream>

    struct A
    {
        ~A(){std::cout << "Deleted\n";}
    };

    A& Get()
    {
      static A & a = *new A;
      return a;
    }

    A& Get2()
    {
      static A a;
      return a;
    }

    int main()
    {
       //Case 1
       Get();
       //Case 2
       Get2();
    }

The output of this program is nothing when calling Get and Deleted when calling Get2.
In other words, resources with nontrivial destructors (commit-on-close file handle for example) will not be destroyed properly at program termination in case 1, but will in case 2.

Graznarak · Answer 5 · 2013-11-22T17:35:35.050

The problem is that a raw new does not specify ownership. If I new up an object and return it, who owns it? Does the creating function/object own it, or does the calling function? If you return smart pointers (std::shared_ptr and std::unique_ptr) you are specifying ownership.

Not specifying ownership is one of the easiest ways to leak memory. I have the hardest time, even with professional programmers, getting people to understand ownership and work with it. This is mostly prevented by using good types (smart pointers) that specify ownership, just by existing.

type* function();    // Unspecified ownership.
                     // Must be well documented and all users must read
                     // and follow the documentation.
std::unique_ptr<type> function(); // Calling function owns returned pointer.
                                  // Single ownership.
std::shared_ptr<type> function(); // Calling function owns returned pointer.
                                  // Shared ownership.  Can have multiple owners.
std::weak_ptr<type> function();   // Calling function references returned pointer.
                                  // Must lock pointer to get owned object, if not deleted.
                                  // Shared ownership.  Can have multiple owners.

These different types of pointers express ownership just by existing unlike raw pointers.

As for new always being wrong. That is an overbroad generalization. std::shared_ptr is created using the global function std::make_shared. As of C++11 there is no std::make_unique, but that will be fixed in C++14. The only way to create a std::unique_ptr is to use new and immediately assign the pointer to a std::unique_ptr.

There are also places where you would want a raw pointer and to manually use new and delete, but they tend to be very low level and most programmers will rarely encounter them.

What really has me cringing about your code is not that you are using new but that you are dereferencing the pointer and assigning it to a reference. It would be almost impossible to guarantee that the destructor will ever be called. It also tends to leak memory, though in the case of assigning to a static variable it will be deallocated at program termination so you aren't really looking at memory leaking.

MyClass& MyClass::MyInstance()
{       
    static MyClass & myLocalVariable = * new MyClass(/*parameters*/);
    return myLocalVariable ;
}

I would prefer to create to have the static variable be by value than by reference. This prevents putting the object on the heap. Depending on MyClass is could also allow the object to be mapped to memory from the executable without having to run any code to initialize it.

MyClass& MyClass::MyInstance()
{
    static MyClass myLocalVariable(/*parameters*/);
    return myLocalVariable ;
}

*new is always wrong. ALWAYS

5 Answers5