3

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; }

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.

Improve this question
0

5 Answers 5

Reset to default
7

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; }
Improve this answer
Sign up to request clarification or add additional context in comments.

8 Comments

That is a great point, i forgot about checking if it can actually add a new action!
This is great, but sadly new actions can be millions for a state, so counting them won't help much, yet I do not see if you release the memory occupied by new_action prior to re-allocating. Is it ok to simply redo new on the same variable ?
@Alex, when you return new_action to the caller, it becomes the caller's responsibility to free. The responsibility goes with the pointer itself. The next time you come into the function, you will get a new pointer.
@paxdiablo ok, but what if the "new_action" must nessaisarily be handled in this function ? Once we get to the point where action is returned, we do not intend to delete it anymore (only serialize it when program exits). Deletion in this case is only if the new_action already exists in the state
@Alex, that's what I've given you. The function will return a new action after it's been added to state. If it can't, you'll get back null and everything will be cleaned up correctly.
|
2

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?

Improve this answer

6 Comments

True, that would be far more sensible. So, I should not be reusing the same memory allocated object without deleting it ?
These objects belong to state, and are stored in a boost::ptr_vector. Howevere they are initialized by Policy ( part of the code above ), so I need to create them there, and then pass them to state(s). I'm allocating them in the loop because they have to be unique, and uniqueness is checked when trying to push them into a state.
@Neil G no not C++0X, C++99 (or whatever is the default for gcc 4.5)
@Alex: Why are they stored in a ptr_vector? Again, this is C++, not Java; you don't have to make everything pointers. And what does uniqueness have to do with their pointer value? Uniqueness is typically defined by the content of the object, not its pointer.
@Alex: FYI, gcc 4.5 does support C++0x with a compiler flag. When you have more time, you might want to see what they've added. It makes writing code like this exception-safe, and painless IMHO.
|
2

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.]

Improve this answer

2 Comments

Isn't it dangerous to delete unused memory ? In the first loop when deleting new_action it essentialy tries to free already freed memory, no ?
The problem with doing your own memory management as in this code is that if addAction throws an exception, it can leak new_action. This code is "too clever". If you're using C++0x, then there is no performance penalty for using a unique_ptr.
2

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 &&
Improve this answer

2 Comments

I could understand a std::unique_ptr (but that requires C++0x, which isn't that widespread), but std::scoped_ptr's cannot transfer ownership. Which is what he's asking for.
@Nicol: You're right. I replaced it with shared_ptr when I thought about it some more.
1

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.

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.