37

Is this valid? An acceptable practice?

typedef vector<int> intArray;

intArray& createArray()
{
    intArray *arr = new intArray(10000, 0);

    return(*arr);
}


int main(int argc, char *argv[])
{

    intArray& array = createArray();

    //..........

    delete &array;

    return 0;
}
Newton Falls
  • 2,148
  • 3
  • 17
  • 22
  • 6
    valid? yes, acceptable? oh god no! why? why would anyone do that? i need to know! – gnzlbg Feb 01 '14 at 10:38
  • @gnzlbg Presumably because this "ensures" that the return value _cannot_ be `null` (though see Billy ONeal's point about exception safety). – Kyle Strand May 26 '15 at 16:08
  • If the vector must be on the heap: use a smart pointer that handles the deallocation automatically. Note also that the return value can't be null because the version of new being used either works or throws. Anyhow, this is very poor API design, having to deallocate memory through a (non-owning) reference is not common practice. And since in practice null references actually work, would you trust whoever wrote this API not to return a null reference? I wouldn't. – gnzlbg May 26 '15 at 22:47

6 Answers6

75

The behavior of the code will be your intended behavior. Now, the problem is that while you might consider that programming is about writing something for the compiler to process, it is just as much about writing something that other programmers (or you in the future) will understand and be able to maintain. The code you provided in many cases will be equivalent to using pointers for the compiler, but for other programmers, it will just be a potential source of errors.

References are meant to be aliases to objects that are managed somewhere else, somehow else. In general people will be surprised when they encounter delete &ref, and in most cases programmers won't expect having to perform a delete on the address of a reference, so chances are that in the future someone is going to call the function an forget about deleting and you will have a memory leak.

In most cases, memory can be better managed by the use of smart pointers (if you cannot use other high level constructs like std::vectors). By hiding the pointer away behind the reference you are making it harder to use smart pointers on the returned reference, and thus you are not helping but making it harder for users to work with your interface.

Finally, the good thing about references is that when you read them in code, you know that the lifetime of the object is managed somewhere else and you need not to worry about it. By using a reference instead of a pointer you are basically going back to the single solution (previously in C only pointers) and suddenly extra care must be taken with all references to figure out whether memory must be managed there or not. That means more effort, more time to think about memory management, and less time to worry about the actual problem being solved -- with the extra strain of unusual code, people grow used to look for memory leaks with pointers and expect none out of references.

In a few words: having memory held by reference hides from the user the requirement to handle the memory and makes it harder to do so correctly.

Dr. Gut
  • 2,053
  • 7
  • 26
David Rodríguez - dribeas
  • 204,818
  • 23
  • 294
  • 489
19

Yes, I think it will work. But if I saw something like this in any code I worked on, I would rip it out and refactor right away.

If you intend to return an allocated object, use a pointer. Please!

Mark Ransom
  • 299,747
  • 42
  • 398
  • 622
  • 2
    Arguably, they might want to wrap it in an appropriate smart pointer, to make it exception-safe. – Steven Sudit Jul 13 '10 at 03:34
  • @Steven Sudit, the appropriate smart pointer would be `auto_ptr`: http://stackoverflow.com/questions/631633/returning-a-pointer-which-is-required-to-be-held-by-a-smart-pointer – Mark Ransom Jul 14 '10 at 18:20
  • Yes, that's certainly the reasonable default. I just didn't want to be overly specific because I'm not up on all of the non-ANSI smart pointers out there, such as the ones in Boost. – Steven Sudit Jul 14 '10 at 19:16
  • 2
    @Steven Sudit, my practice has always been to return raw pointers because it allows the greatest flexibility. But the answer in that question I linked to demonstrates that auto_ptr has the same flexibility, even considering the choices in Boost. The fact that it's part of the standard makes it a no-brainer. – Mark Ransom Jul 14 '10 at 19:58
  • I can't fault your reasoning. The part I like about `auto_ptr` is that it's bulletproof: if the caller forgets that there's a return value, it just gets deleted. Hard to beat that. – Steven Sudit Jul 14 '10 at 20:08
  • 2
    Of course, nowadays it's worth mentioning that `auto_ptr` was deprecated in favour of its superior replacements, `unique_ptr` or `shared_ptr` depending on situation. – underscore_d Dec 05 '15 at 22:46
13

It's valid... but I don't see why you'd ever want to do it. It's not exception safe, and std::vector is going to manage the memory for you anyway. Why new it?

EDIT: If you are returning new'd memory from a function, you should return the pointer, lest users of your function's heads explode.

Billy ONeal
  • 104,103
  • 58
  • 317
  • 552
  • How is this not exception-safe? The caller is none other than the `main()` function, right? – Tanz87 Aug 21 '16 at 20:31
  • 1
    @Tanz: In the specific case of main() you're "safe" because you know any exception not handled in main is going to be unhandled (resulting in `termninate()` in the event an exception is thrown). However (1) you should still use objects to manage resources, even in main, for consistency if nothing else, and (2) the OP's question is general, not specific to main(). – Billy ONeal Aug 25 '16 at 05:00
5

Is this valid?

Yes.

An acceptable practice?

No.

This code has several problems:

  • The guideline of designing for least surprising behavior is broken: you return something that "looks like" an object but must be deleted by the client code (that should mean a pointer - a reference should be something that always points to a valid object).

  • your allocation can fail. Even if you check the result in the allocating function, what will you return? An invalid reference? Do you rely on the allocation throwing an exception for such a case?

As a design principle, consider either creating a RAII object that is responsible for managing the lifetime of your object (in this case a smart pointer) or deleting the pointer at the same abstraction level that you created it:

typedef vector<int> intArray;

intArray& createArray()
{
    intArray *arr = new intArray(10000, 0);

    return(*arr);
}

void deleteArray(intArray& object)
{
    delete &object;
}

int main(int argc, char *argv[])
{
    intArray& array = createArray();
    //..........
    deleteArray(array);
    return 0;
}

This design improves coding style consistency (allocation and deallocation are hidden and implemented at the same abstraction level) but it would still make more sense to work through a pointer than a reference (unless the fact that your object is dynamically allocated must remain an implementation detail for some design reason).

utnapistim
  • 26,809
  • 3
  • 46
  • 82
  • Why is allocation failure a problem? If the allocation fails that function _returns_ an exception (that version of operator new throws). – gnzlbg May 26 '15 at 22:51
0

It is valid because compiler can compile and run successfully. However, this kind of coding practices makes codes more harder for readers and maintainers because of

  • Manual memory management
  • Vague ownership transfer to client side

But there is a subtle point in this question, it is efficiency requirement. Sometimes we can not return pass-by value because object size might be too big, bulky as in this example (1000 * sizeof(int)); For that reason; we should use pointers if we need to transfer objects to different parts of our code. But this doesn't means above implementation is acceptable because for this kind of requirements, there is very useful tool, it is smart-pointers. So, design decision is up to programmer but for this kind of specific implementation details, programmer should use acceptable patterns like smart-pointers in this example.

Validus Oculus
  • 2,756
  • 1
  • 25
  • 34
0

It will work but I'm afraid it's flat-out unacceptable practise. There's a strong convention in the C++ world that memory management is done with pointers. Your code violates this convention, and is liable to trip up just about anyone who uses it.

It seems like you're going out of your way to avoid returning a raw pointer from this function. If your concern is having to check repeatedly for a valid pointer in main, you can use a reference for the processing of your array. But have createArray return a pointer, and make sure that the code which deletes the array takes it as a pointer too. Or, if it's really as simple as this, simply declare the array on the stack in main and forego the function altogether. (Initialization code in that case could take a reference to the array object to be initialized, and the caller could pass its stack object to the init code.)

Owen S.
  • 7,665
  • 1
  • 28
  • 44