Confusion with pointers in a deleting bst function

Question

So this is my code. It should delete a sub-tree of a BST, "buscar" function is the search function, then it passes to "eliminar" the correct pointer and then I got confused with pointers. Should I free(*arbol) or free(arbol)?

struct s_nodo
{
    struct s_nodo * der;
    struct s_nodo * izq;
    int valor;
};
typedef struct s_nodo * t_nodo;

void buscar (t_nodo * arbol,int num)
{
    if(*arbol != NULL)
    {
        if((*arbol)->valor == num)
            eliminar(arbol);
        else
        {
            if((*arbol)->valor > num)
                buscar(&(*arbol)->izq,num);
            else if((*arbol)->valor < num)
                buscar(&(*arbol)->der,num);
        }
    }
    else
    {
        return;
    }    
}

void eliminar (t_nodo * arbol)
{
    if(*arbol != NULL)
    {
        eliminar(&(*arbol)->izq);
        eliminar(&(*arbol)->der);
        free(*arbol); //problematic line
        *arbol = NULL;
    }
}

Regarding _Should i free(*arbol) or free(arbol)?_: Where did you `m(c)alloc()` either of them? If you did not use an allocation function to put space on the heap, you will not need to use `free()` to free memory. — ryyker, Feb 11 '14 at 15:33
Note that the `else { return; }` is really not needed; there's an implicit `return;` for the `if` clause just after that anyway. To answer your question, it seems like it should be `*arbol` that you pass to `free()`. — Jonathan Leffler, Feb 11 '14 at 15:37
Note also the utter lack of clarity typedef'd pointer types bring to code never ceases to make me wonder why people do it. (and +1 to Jonathan). — WhozCraig, Feb 11 '14 at 15:43
@WhozCraig - regarding _utter lack of clarity typedef'd pointer types bring..._ Although I agree this usage is hard to follow, there _are_ cleaner ways and appropriate times to implement typedef'd pointer types. — ryyker, Feb 11 '14 at 15:49
@ryyker there are only **two** times a typedef'd pointer is ok, and only one of those is it actually *appropriate*.. A handle-api abstraction (exposing a pointer as an opaque handle from a library API) is both ok and appropriate. The only other place it does anything arguably "useful" is avoiding a mistaken missed pointer-decl during variable declaration. I.e. `Type* a,b;` vs `TypePtr a,b;`. The latter I consider personally useless, since the context of the code usage itself will generally fail to compile because you decl'd wrong. Outside of those, it is pointless (pun intended). — WhozCraig, Feb 11 '14 at 16:02
I'd add to the list of OK typedef'd pointers "it is OK to typedef function pointers". See the discussion at [Typedef pointers — a good idea?](http://stackoverflow.com/questions/750178/typedef-pointers-a-good-idea) and related questions. — Jonathan Leffler, Feb 11 '14 at 16:07
@JonathanLeffler touche, I had data-only on the mind (still early here =P), and in that I can honestly only think of *one* decent example. A pointer to function would make good sense for consistency across a slurry of api prototypes and implementations needing identical callback signatures. great article, btw, the selected answer not-coincidentally being the opaque handle mantra mentioned earlier. — WhozCraig, Feb 11 '14 at 16:12

score 1 · Answer 1 · edited May 23 '17 at 12:05

Your code is fine with free(*arbol). An SSCCE (Short, Self-Contained, Correct Example) demonstrates this:

#include <stdio.h>
#include <stdlib.h>

struct s_nodo
{
    struct s_nodo *der;
    struct s_nodo *izq;
    int valor;
};
typedef struct s_nodo *t_nodo;

static void eliminar(t_nodo *arbol)
{
    if (*arbol != NULL)
    {
        eliminar(&(*arbol)->izq);
        eliminar(&(*arbol)->der);
        free(*arbol); //problematic line
        *arbol = NULL;
    }
}

static void buscar(t_nodo *arbol, int num)
{
    if (*arbol != NULL)
    {
        if ((*arbol)->valor == num)
            eliminar(arbol);
        else
        {
            if ((*arbol)->valor > num)
                buscar(&(*arbol)->izq, num);
            else if ((*arbol)->valor < num)
                buscar(&(*arbol)->der, num);
        }
    }
}

static void p_inorder(t_nodo node)
{
    if (node != 0)
    {
        p_inorder(node->izq);
        printf(" %d", node->valor);
        p_inorder(node->der);
    }
}

static void print(char *tag, t_nodo root)
{
    printf("%s: ", tag);
    p_inorder(root);
    putchar('\n');
}

int main(void)
{
    t_nodo left = malloc(sizeof(*left));
    left->valor = 3;
    left->izq = 0;
    left->der = 0;
    t_nodo right = malloc(sizeof(*right));
    right->valor = 6;
    right->izq = 0;
    right->der = 0;
    t_nodo root = malloc(sizeof(*root));
    root->valor = 5; 
    root->izq = left;
    root->der = right;

    print("Before", root);
    buscar(&root, 3);
    print("After", root);
    free(right);
    free(root);
    return 0;
}

When run on Ubuntu 13.10 with valgrind, you get a clean bill of health.

==6217== Memcheck, a memory error detector
==6217== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==6217== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==6217== Command: ./bst3
==6217== 
Before:  3 5 6
After:  5 6
==6217== 
==6217== HEAP SUMMARY:
==6217==     in use at exit: 0 bytes in 0 blocks
==6217==   total heap usage: 3 allocs, 3 frees, 72 bytes allocated
==6217== 
==6217== All heap blocks were freed -- no leaks are possible
==6217== 
==6217== For counts of detected and suppressed errors, rerun with: -v
==6217== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 2)

I also ran a variant of the program which deleted value 5 (and did not explicitly free right or root); the 'After' tree was empty and valgrind was equally happy.

I also completely agree with the comment by WhozCraig that embedding a pointer in the typedef for a non-opaque type is apt to be confusing — I would have written:

typedef struct Node Node;

since the structure tag name space is separate from the ordinary identifiers name space, and would never write:

typedef struct Node *pNode;

Think of FILE in <stdio.h>; you always use FILE * everywhere; I'd use Node * everywhere I passed a pointer to a Node, and Node ** when relevant.

See also typedef pointers — a good idea?

What happens with free(arbol) because that works on my PC?

`valgrind` gets justifiably upset:

==6284== Memcheck, a memory error detector
==6284== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==6284== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==6284== Command: ./bst3
==6284== 
Before:  3 5 6
==6284== Invalid free() / delete / delete[] / realloc()
==6284==    at 0x4C2B60C: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6284==    by 0x400C23: eliminar (bst3.c:18)
==6284==    by 0x400636: main (bst3.c:28)
==6284==  Address 0x51fc108 is 8 bytes inside a block of size 24 alloc'd
==6284==    at 0x4C2A2DB: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6284==    by 0x4005D5: main (bst3.c:66)
==6284== 
==6284== Invalid write of size 8
==6284==    at 0x4011BB: eliminar (bst3.c:19)
==6284==    by 0x400636: main (bst3.c:28)
==6284==  Address 0x51fc100 is 0 bytes inside a block of size 24 free'd
==6284==    at 0x4C2B60C: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6284==    by 0x4011BA: eliminar (bst3.c:18)
==6284==    by 0x400636: main (bst3.c:28)
==6284== 
==6284== Invalid free() / delete / delete[] / realloc()
==6284==    at 0x4C2B60C: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6284==    by 0x4011CA: eliminar (bst3.c:18)
==6284==    by 0x400636: main (bst3.c:28)
==6284==  Address 0x7fefffda0 is on thread 1's stack
==6284== 
After: 
==6284== 
==6284== HEAP SUMMARY:
==6284==     in use at exit: 48 bytes in 2 blocks
==6284==   total heap usage: 3 allocs, 3 frees, 72 bytes allocated
==6284== 
==6284== LEAK SUMMARY:
==6284==    definitely lost: 48 bytes in 2 blocks
==6284==    indirectly lost: 0 bytes in 0 blocks
==6284==      possibly lost: 0 bytes in 0 blocks
==6284==    still reachable: 0 bytes in 0 blocks
==6284==         suppressed: 0 bytes in 0 blocks
==6284== Rerun with --leak-check=full to see details of leaked memory
==6284== 
==6284== For counts of detected and suppressed errors, rerun with: -v
==6284== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 2 from 2)

This was for the second case I tested: the tail of the main() was:

    print("Before", root);
    buscar(&root, 5);
    print("After", root);
    return 0;
}

Jonathan, after the free(*arbol) in my code there is a *arbol= NULL, it is needed? — csjr, Feb 11 '14 at 16:01
Yes, it is needed. Otherwise, the pointer to the node is not nulled, so the next time something traverses the tree (e.g. a print function) it will be accessing freed memory, which is a bad idea. — Jonathan Leffler, Feb 11 '14 at 16:06
Ok, Jonathan, did u test the same function writing free(arbol) in my pc it works the same, with the pointer or without it — csjr, Feb 11 '14 at 16:08
@SebastianC it is worth noting the pointer-to-pointer usage here isn't a whim. The algorithms in-play become *significantly* more concise when you don't have to tote around a bunch of temporary pointers to nodes just to access said-node's left or right child pointers. The algorithm actually uses the nodes *in the tree* (by their addresses, so pointer-to-pointer) for enumeration and deletion management. This is *extremely* helpful. — WhozCraig, Feb 11 '14 at 16:21
Apologies for inverting the usage of `der` 'derecha' ('left' in Spanish) and `izq` 'izquierda' ('right'). — Jonathan Leffler, Feb 11 '14 at 17:21

Confusion with pointers in a deleting bst function

1 Answers1