Search code examples
cmemory-leaksmallocfreec99

How to call free() properly?


Kindly read the whole post since it includes small details which are highly important.

As known by C we should take care of incidents where malloc fails, for that case I created a function called destroyList() whose job is to take a pointer to Node and destroy it one by one. But my function isn't being called correctly...

I tried to call it with ptr, merged_out and *merged_out (The last one was a suggestion from a member of the community) but nothing seems to work.

Why is that? the function sometimes receives NULL, Empty Lists or some random values.

Can someone please help me fix this issue and let me understand of what is going on?

typedef struct node_t {
    int x;
    struct node_t *next;
} *Node;

void destroyList(Node ptr) {
    while (ptr) {
        Node toDelete = ptr;
        ptr = ptr->next;
        free(toDelete);
    }
}

Main Function:

ErrorCode mergeSortedLists(Node list1, Node list2, Node *merged_out) {
    if (!list1 || !list2) {
        return EMPTY_LIST;
    }
    if (!isListSorted(list1) || !isListSorted(list2)) {
        return UNSORTED_LIST;
    }
    if (!merged_out) {
        return NULL_ARGUMENT;
    }
    Node ptr = NULL;
    int total_len = getListLength(list1) + getListLength(list2);
    for (int i = 0; i < total_len; i++) {
        int min = getMin(&list1, &list2);
        ptr = malloc(sizeof(*ptr));
        *merged_out = ptr;
        if (!ptr) {
            destroyList(*merged_out);
            *merged_out = NULL;
            return MEMORY_ERROR;
        }
        ptr->x = min;
        ptr->next = NULL;
        merged_out = &ptr->next;
    }
    return SUCCESS;
}

This is how the function should be called:

Node merged_actual = NULL;
ErrorCode merge_status = mergeSortedLists(list1, list2, &merged_actual);

Note: getMin() gets the minimum value and advances the pointer of the list which has that min value to the next node.


Solution

  • Start after those if checks.

        Node ptr=NULL,last;
        /* find out current tail of the list */
        if (*merged_out!=NULL){
            last=*merged_out;
            while (last->next!=NULL){
                last=last->next;
            }
        }
        int total_len = getListLength(list1) + getListLength(list2);
        for (int i = 0; i < total_len; i++)
        {
            int min = getMin(&list1, &list2);
            ptr = malloc(sizeof(*ptr));
            if (!ptr)
            {
                destroyList(*merged_out);
                *merged_out=NULL;
                return MEMORY_ERROR;
            }
            ptr->x = min;
            ptr->next = NULL;
            /* link ptr onto the list */
            if (*merged_out==NULL){
                /* if the list is empty, make ptr the head of the list */
                *merged_out=ptr;
                last=*merged_out;
            }
            else{
                last->next = ptr;
                last = ptr;
            }
        }
    

    Please try not to copy and paste this block of code. It may or may not be correct, but try to understand what it did: iterate each time the function is called, in an effort to put last to point at the last element of the list. Therefore merged_out can always point to the head.