4

I just want to know if this is a bad practice to do.

for(int i=0;i<1000;i++)
    happyFunction(new Car());

The new car() object should live after calling it from happyFunction and later it should be destroyed . Now is that ok to do. I mean shouldn't I delete the memory allocated for that instance?

Example to make it more clear.

int i = 0;
for (i=0;i<1000;i++){
    new car();
}

Is this a good practice to do? Should I delete the memory allocation? I hope my question is clear. thank you.

Francesco Boi
  • 5,497
  • 8
  • 54
  • 83
user1580731
  • 51
  • 1
  • 3
  • 8
    you should better read some c++ book – Andrew Aug 07 '12 at 11:23
  • 1
    possible duplicate of [In C++, why should `new` be used as little as possible?](http://stackoverflow.com/questions/6500313/in-c-why-should-new-be-used-as-little-as-possible) – KillianDS Aug 07 '12 at 11:25
  • Well I guess you need to be a bit more specific. I say in general it's bad practice, but sometimes you can do it for convenience when it does not matter. – ygram Aug 07 '12 at 11:29
  • It's really, really difficult to get this right. For example, if `happyFunction` (or a function it calls) throws an exception, it may be unable to ever `delete` the `Car`. So this forced `happyFunction` to ensure it `delete`s in all cases (having to catch and re-throw exceptions), which makes things really ugly and easy to get wrong. (Even if you don't use exceptions, it's still hard to avoid leaks. Use smart pointers.) – David Schwartz Aug 07 '12 at 11:52
  • You are NOT passing a new object in your example, you are passing the pointer to a new object, which the new operator returns. If your happyFunction doesn't deal with the deallocation of the object, you have no way of deleting that object from memory. IT IS NEVER GOOD PRACTICE to create dynamically allocated objects without having a pointer to either delete or pass to a parent object to deal with later. – dtech Aug 07 '12 at 11:55
  • 1
    Unless you need the Car object for later on, you should omit the dynamic allocation and simply create a static object, which will be temporary created by the function call, and when the function is over it will be deleted. – dtech Aug 07 '12 at 11:59
  • @ddriver - except when it is, eg. 'new PoolThread(poolQueue);'. No reference to the PoolThread is either needed or desirable. In fact, any object that is expected to last the lifetime of the app and is accessed via. ctor parameters may not need any ref stored. – Martin James Aug 07 '12 at 18:01

3 Answers3

7
happyFunction(new Car()); 

It's not bad practice in itsself (although almost certainly is wrong), the memory could be deleted inside the function. But that would be confusing so it's really not the best idea.

Also although it's safe with one parameter, if it was like this

happyFunction(new Car(), new Thing()); 

And one of the news threw an exception after the other new executed, the would be no way to free the memory so it's not safe.

You always have to free memory yourself in c++ so your second example leads to a big memory leak. There are classes such as unique_ptr and shared_ptr to help you manage it without having to write a delete yourself, you can find any number of tutorials online about them

jcoder
  • 28,098
  • 16
  • 76
  • 120
2

There are two possibilities:

  1. happyFunction is supposed to take ownership of the pointer, and the caller never worries about it. In this case, it would be more sensible to write

    void happyFunction(std::unique_ptr<Car> &&car)
    {
        // car is mine now, and is automatically destroyed when I return
        // unless I explicitly request otherwise
    }
    
    void caller()
    {
        happyFunction(std::unique_ptr<Car>(new Car));
        // the new Car is immediately handed over to the unique_ptr
        // and I don't have to worry about leaks
    }
    
  2. happyFunction is only supposed to use the pointer: the caller retains control and ownership. In this case, it would be better to pass a reference so there is no suggestion that ownership is transferred

    void happyFunction(Car &car)
    {
        // car is still owned by the caller,
        // I just get a reference to use in here
    }
    
    void automatic_caller()
    {
        Car car;
        happyFunction(car);
        // car is always owned by me, and is
        // automatically destroyed at the end of this scope
    }
    
    // alternatively (only if car should live longer than the enclosing scope)
    void dynamic_caller()
    {
        std::unique_ptr<Car> car(new Car);
        // or get pointer from somewhere else ...
        // or get shared_pointer, etc. etc.
        happyFunction(*car);
        // again car is destroyed here unless we do something special
    }
    
Useless
  • 55,472
  • 5
  • 73
  • 117
  • I realize this is a bit old, but I'd like to ask if the caller can still retain ownership of the allocated object if it is called in the way that the OP described. Ie, if he uses `happyFunction(new Car());`, will the caller still be able to `delete` it? I ask because you say `it would be better to pass a reference` in #2, implying that an alternative (possibly the original method) exists. – Hydronium Mar 15 '13 at 14:41
  • In your example, the caller doesn't _have_ a pointer to the object (since it never assigns the result of `new` to a variable, it only gets sent to the function call). So, there is _no way_ for the caller to delete the object. – Useless Mar 15 '13 at 17:36
  • ...that seems dangerous. Is there a reason compilers/the C++ standard allows this, since it seems like something that should never be done? – Hydronium Mar 15 '13 at 17:38
  • That (ex. 1 above) is how you explicitly pass ownership, which is something you might genuinely want to do. If you don't want to explicitly pass ownership and instead keep it in the caller ... just do that instead (as in example 2 above). – Useless Mar 15 '13 at 17:50
  • I meant the use of `new` as a function argument. Though, it's somewhat silly now as a question as I forgot that if you have control over the called code, you could deal with it that way. I was thinking purely from the thought that you didn't know what the callee expected/had no control over the called function. Thanks. – Hydronium Mar 15 '13 at 17:54
1

Caller can retain ownership of the pointer to new Car() if your happyFunction() returns pointer to that memory created by new Car().

Consider following code:

#include <string>
#include <iostream>

using std::string;
using std::cout;

class Car
{
public:
    string s;
    Car(string str):s(str) {}
};

Car* happyFunction(Car *pCar)
{
    // do something with data pointed to by pCar
    return pCar;
};

int main()
{
    // outer pointer to memory allocated by new operator
    Car *pCar = happyFunction(new Car("test"));
    // pCar still points to valid object even after happyFunction() content
    // went out of scope
    cout << pCar->s << "\n";

    // release pCar memory outside the happyFunction()
    delete pCar;

    return 0;
}
Petr Javorik
  • 1,179
  • 12
  • 22