C++ Heap allocation and reuse of memory

I've got this small snippet:

Action* newAction(Agent& a, ACTION_MODE& m)
{
  Action *new_action;
  do
  {
    new_action = new Action(a,m);
  }
  while (!state->addAction(new_action));
  return new_action;
}

state->addAction(Action *a); will return true if *a was added, false if *a not added (checks to see if *a already exists).

Now, I know that goto's are considered evil from lots of people, so, In the above snippet, re-allocating new_action in every loop, without deleting it, isn't that wrong ?

Wouldn't the following snippet be more "sensible" ?

retry :
    Action *new_action = new Action(a,m);
    if (state->addAction(new_action))
    {
      return new_action;
    }
    else
    {
      delete new_action;
      goto retry;
   开发者_如何学C }

I'm sorry if this seems elementary but it is something I've wondered about for some time now. What is correct, to delete the memory and then re-allocate, or can I re-allocate instantly ?

EDIT:

Would that be better ?

Action* newAction(Agent& a, ACTION_MODE& m)
{      
  // state will only allow an action to be added if it does not already exist
  Action *new_action = new Action(a,m);

  if (!new_action) return 0;

  while (!state->addAction(new_action))
  {
    delete new_action;
    new_action = new Action(a,m);
  }

  return new_action;
}

The caller of this function expects a new action which has been already added to state, so therefore deletion must take place in here.

You have a potential memory leak in that code in that you are creating a new action every time through the loop but, if it fails to add, you don't free it. Better would be to move the action creation outside of the loop with something like:

Action *newAction (Agent& a, ACTION_MODE& m) {
    Action *new_action = new Action(a,m);
    while (!state->addAction(new_action))
        ;
    return new_action;
}

This is more efficient than continuously deleting and re-creating the action and should be used in preference.

I've also fixed the return type so that the compiler won't complain, and you should probably introduce some sort of failure strategy if you are never able to add the action.

Something like:

Action *newAction (Agent& a, ACTION_MODE& m) {
    // Try to make action, return null if no go.

    Action *new_action = new Action(a,m);
    if (new_action == 0)
        return 0;

    // Try a limited number of times to add action to state.

    int limit = 10;
    while (!state->addAction(new_action)) {
        if (--limit == 0)
            break;
        // Optional sleep if desired.
    }

    // If that didn't work, delete action and return null.

    if (limit == 0) {
        delete new_action;
        return 0;
    }

    // Otherwise the action was added, return it.

    return new_action;
}

Wouldn't the following snippet be more "sensible" ?

No. This would be far more reasonable:

do
{
  new_action = new Action(a,m);
}
while (!state->addActionOrDelete(new_action));

Where addActionOrDelete does exactly what it says. If you are going to transfer ownership of a pointer to another function, then that other function must take responsibility for the ownership of that pointer. If it cannot add it, then it must delete it.

But to be honest, I don't understand why you are allocating these Action objects in a loop anyway. Is the state of state going to change, such that adding an action didn't work before, but it may work later? If so, wouldn't it make more sense to have a state->addAction that will block until the action can be added?

And why the allocation at all? This looks like Java or C#-style code. Just do this:

while(!state->addAction(Action(a,m))) ;

That will create temporary Action objects. No need for heap allocation. Or even better:

Action theAction(a, m);
while(!state->addAction(theAction)) ;

Are these objects hard to copy?

If you want to retain your current code context then, do 2 small changes:

  Action *new_action = 0;  //<-- (1) initialize to 0
  do
  {
    delete new_action;   // (2) delete the previous pointer
    new_action = new Action(a,m);
  }
  while (!state->addAction(new_action));

Edit: I realized from @paxdiablo's answer that you actually don't need to keep allocating and deallocating the memory. Simply do this, as he suggested:

Action *new_action = new Action(a,m);
while(!state->addAction(new_action));
return new_action;

[Note: Choose his answer, if you are going to accept mine.]

Using a shared_ptr or unique_ptr would be more sensible. You shouldn't be doing your own memory allocation in C++ anymore.

In any case, I don't think that this is a good place to use a goto. You are implementing looping, and that's exactly what a loop construct is for.

Here's how to do it with unique_ptr:

unique_ptr<Action> new_action;
do {
  new_action.reset(new Action(a,m));
} while (!state->addAction(std::move(new_action)));  // accept by &&

I would write it like this:

for ( ; ; ) {
    Action* new_action = new Action(a, m);
    if (state->addAction(new_action)) return new_action;
    delete new_action;
}

If you dislike returning from the middle of a loop, change it to break, but it means you have to move the declaration of new_action outside the loop as well.

I always try to pair new and delete. Having one in this function, and one in the function it calls seems like a bad practice to me.