Proper Way to Cleanup in a Function with Multiple Return Points

Question

I have a recursive search algorithm, and I want to clean up my pointers after each call. However, I return in so many locations, it seems sloppy to put a delete or free before every one.

Is there a better way? Does me freeing them all at return of the function mean I should just allocate them on the stack instead of in the heap?

Note this is a parallel search (not shown in code), but the caller will never return before its children. Does this have any additional pitfalls for using the stack?

Example Code (Don't worry about the algorithm here):

//create a new struct state (using new), initialize and return (C style)
new_state()

free_list(state* node)//free a list

double minimax(state* node, state* bestState) {

    if (base_case) {
        return;
    }

    state* gb = new_state(); //single node
    state* children = new_state(); //head of list

    generate_children(children); //fill list

    state* current = children; //traverse node

    //recurse on child
    double result = -minimax(current, gb);

    if (case1) {
        free(gb);
        free_list(children);
        return;
    }
    if (case2)  {
        //do stuff
    }

    while(current != NULL){
        result = -minimax(current, gb);
        if (case1) {
            free(gb);
            free_list(children);
            return;
        }
        if (case2)  {
            //do stuff
        }
        current = current->next;
    }
    free(gb);
    gb = NULL;

    //More stuff (with children but not gb)
    free_list(children);
    return;
}

This is why you use RAII and smart pointers. They cleanup themselves leaving you to work on logic instead of cleanup. — NathanOliver, May 04 '17 at 19:57
@NathanOliver can I customize smart pointer destructors? For example, I need the whole list that `children` points to be removed when the scope is exited. — River, May 04 '17 at 20:02
Sure. You can give them a custom deleter that will be ran when it goes out of scope. You can also just build a class type that encapsulates the list and clean it up with the destructor. If you do that then the smart pointer will call the objects destructor when it goes out of scope. — NathanOliver, May 04 '17 at 20:04
@River, you can also create a macro `scope_exit`. I made [a working version](https://codereview.stackexchange.com/questions/145801/scope-exit-macro), but it only works for one `scope_exit` in a current scope. It is RAII under the hood anyway. — Incomputable, May 04 '17 at 20:07
`gb = NULL;` -- This line of code near the end of your function doesn't really affect anything. It can be removed. — PaulMcKenzie, May 04 '17 at 20:09
@PaulMcKenzie That's just to make sure I don't use `gb` after it's `free`'d. I heard it was [good practice](http://stackoverflow.com/questions/1025589/setting-variable-to-null-after-free). — River, May 04 '17 at 20:11

Mike Nakis · Answer 1 · 2017-05-04T20:51:44.123

1

However, I return in so many locations, it seems sloppy to put a delete or free before every one.

Yes it does.

Is there a better way?

Yes. Smart pointers are a better way. But if you do not want to drop what you are doing, and learn how to use smart pointers before you can continue, (it can be hard the first time,) keep reading further down.

Does me freeing them all at return of the function mean I should just allocate them on the stack instead of in the heap?

Yes, you could do that. It would also perform better. But it will not work if you are planning on allocating a lot of memory.

Note this is a parallel search (not shown in code), but the caller will never return before its children. Does this have any additional pitfalls for using the stack?

The pitfalls are the same. With parallel code, you have to be careful.

There are many ways to avoid this problem. Smart pointers and stack allocation have already been mentioned.

Another way is to have only one exit point. This can get clunky at times, because, for example, it would mean that you would have to set a flag within your loop right before breaking out of it so as to know whether it terminated successfully or due to an error.

Another way is to allocate your pointers in function A, call function B to do the actual work, (passing it the allocated pointers,) and then once function B returns to function A, free the pointers.

edited May 04 '17 at 20:51

answered May 04 '17 at 20:11

Mike Nakis

56,297
11
110
142

Isn't it dangerous to have `children` on the stack though? It points to further nodes down a list that are on the heap. If it gets removed as the scope leaves, doesn't the rest of the list dangle? – River May 04 '17 at 20:13
As much as it pains we to say it you can also use the good ol' `goto cleanup` trick. In all exit points you use `goto cleanup` and then at the end of the function make a `cleanup` label and then do the cleanup code and return all in one place. – NathanOliver May 04 '17 at 20:15
@NathanOliver that would actually work fairly well. Only problem is I'm returning different variables at each return statement. – River May 04 '17 at 20:21
@NathanOliver I share the pain and also the acknowledgement that this is in fact a viable solution and it can even result in more understandable code in certain cases. – Mike Nakis May 04 '17 at 20:21
@River You're calling `delete` regardless in your current code. Thus you're essentially doing right now what a stack based `children` would do. – PaulMcKenzie May 04 '17 at 20:23
@PaulMcKenzie what? I'm calling a custom function that `free`s the whole list. `free_list(children)` – River May 04 '17 at 20:26
I am going by the code you posted. Also, that sounds like a flaw to me, where if you have a local list, it doesn't know how to destroy itself when it goes out of scope. Sounds like you're doing `C`-like coding -- Imagine a string class where the user has to say `free_string()` whenever the string object leaves scope. That gets frustrating and error prone pretty quick. – PaulMcKenzie May 04 '17 at 20:32
@PaulMcKenzie `free_list` is in the code I posted... But yes this is a lot of C-style code, as it was originally a C program. I was more confirming that the dangling is an issue with the stack option, which to which your answer is obviously yes =) – River May 04 '17 at 20:38

score 1 · Accepted Answer · answered May 04 '17 at 20:30

Here is a small sample of RAII:

First we have a struct that simply stores your items.

struct FreeAll
{
    state* gb;
    state* children;
    FreeAll(state* g, state* c) : gb(g), children(c) {}
    ~FreeAll() { free(gb); free(children); }
};

Note that on destruction, the free() is called on both items. How to use it?

double minimax(state* node, state* bestState) 
{
    if (base_case) {
        return;
    }

    state* gb = new_state(); //single node
    state* children = new_state(); //head of list

    // Initialize our small RAII object with the above 
    // pointers   
    FreeAll fa(gb, children);

    generate_children(children); //fill list
    state* current = children; //traverse node
    //recurse on child
    double result = -minimax(current, gb);

    if (case1) {
        return;
    }
    if (case2)  {
        //do stuff
    }

    while(current != NULL){
        result = -minimax(current, gb);
        if (case1) {
            return;
        }
        if (case2)  {
            //do stuff
        }
        current = current->next;
    }
    //More stuff (with children but not gb
    return;
}

The local variable fa is a FreeAll type. When this local goes out of scope, the destructor of fa is called which calls free on both the pointers that were stored in the struct. Also note the lack of any code at the return points to free the memory. This will be done by fa when it goes out of scope.

Note this is a simple example, and has none of the sophistication as other methods mentioned, but it gives you the basic gist of the RAII paradigm.

There's an error here, if `new_state()` raises an exception when creating the `children` state then the `gb` state will be leaked. Raw pointers should appear only for references to someone else's data. If it's your own data then `auto gb = make_unique();` should be used to ensure you never have unsafe data. Because `make_unique` follows the RAII pattern it will serve the requested purpose. — codeshot, Jun 23 '19 at 09:52

score 0 · Answer 3 · answered Jun 23 '19 at 09:46

0

ScopeGuard does the job for you.

https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Scope_Guard

void your_function()
{
     Scope_guard const final_action = []{
            free(gb);
            free_list(children);};
     // your code here
 };

answered Jun 23 '19 at 09:46

bugs king

566
5
13

Proper Way to Cleanup in a Function with Multiple Return Points

3 Answers3