Should a function return a "new" object

Question

Should a function to return a pointer to memory allocated on the heap?

In other words, which of the following methods is more "correct"?

// Method #1
Object* getObject1() {
    return new Object();
}

// Method #2
std::shared_ptr<Object> getObject2() {
    return std::make_shared<Object>();
}

int main() {
    // Usage of method #1
    Object* o1 = getObject1();
    o1->doSomething();
    delete o1; // object has to be deleted by user

    // Usage of method #2
    std::shared_ptr<Object>& o2 getObject2(); // has to be kept in shared_ptr
    o2.get()->doSomething();
    // object is deleted when o2 destructs
}

I imagine the first method is probably faster, but the second method does not require the user to delete the object.

Solution 1

Of the two, the second would be preferred. Naked pointers should always be your last choice. But ideally, you would just return Object by value. Failing that, a unique_ptr would be better than a shared_ptr unless you really do need shared ownership.

I imagine the first method is probably faster.

That's probably true with shared_ptr. But with unique_ptr, the compiler can optimize. There's no benefit that pays you back for the risk of manual deletion.

Solution 2

Neither option is really that great.

By default, the best thing you can do in C++ is this:

Object getObject1() {
    return Object();
}

If you really have to use dynamic allocation, then your first choice should be returning a std::unique_ptr by value:

std::unique_ptr<Object> getObject1() {
    return std::make_unique<Object>();
}

See "GotW #90 Solution: Factories" by Herb Sutter for a nice article on that subject. Among other things, Sutter says:

• Returning unique_ptr expresses returning unique ownership, which is the norm for a pure “source” factory function.
• unique_ptr can’t be beat in efficiency—moving one is about as cheap as moving/copying a raw pointer.

As for your specific questions:

I imagine the first method is probably faster,

std::shared_ptr involves locking in order to be thread-safe, so it may potentially decrease performance. However, it should not really matter. Dynamic allocation is typically always slower than the alternative, so you typically do not want to use it where micro performance matters (such as in a tight loop or in some part of the code which is executed many times per second). C++ discourages you from creating many small objects on the free store. So a function like this should not be able to be a bottleneck in the first place.