0

I have a function in C that adds a new Question to the head of a singly linked list:

int AddQuestion()
{
    unsigned int aCount = 0;
    Question* tempQuestion = malloc(sizeof(Question));

    tempQuestion->text = malloc(500);

    fflush(stdin);
    printf("Add a new question.\n");
    printf("Please enter the question text below:\n");
    fgets(tempQuestion->text, 500, stdin);
    printf("\n");
    fflush(stdin);
    printf("How many answers are there?: ");
    scanf("%u", &tempQuestion->numAnswers);
    fflush(stdin);
    tempQuestion->answers = malloc(sizeof(Answer*) * tempQuestion->numAnswers);
    for (aCount = 0; aCount < tempQuestion->numAnswers; aCount++)
    {
        tempQuestion->answers[aCount] = malloc(sizeof(Answer));
        tempQuestion->answers[aCount]->content = malloc(250);
        printf("Enter answer #%d: \n", (aCount + 1));
        fgets(tempQuestion->answers[aCount]->content, 250, stdin);
        fflush(stdin);
        printf("Is it correct or wrong? correct = 1, wrong = 0: ");
        scanf("%u", &tempQuestion->answers[aCount]->status);
        fflush(stdin);
    }

    tempQuestion->pNext = exam.phead;
    exam.phead = tempQuestion;

    printf("\n");
    fflush(stdin);

    return 1;
}

As you can see, I am using malloc() to allocate the space I need for the new question. However, if I try to call free() on tempQuestion, it removes the question from the exam. I do not want to remove the question unless the question is deleted or the program terminates.

I have a cleanup function that is supposed to free() up all the used memory, but it does not free up tempQuestion in the addQuestion() function.

void CleanUp()
{
    unsigned int i = 0;
    Question* tempQuestion = NULL;

    if (exam.phead != NULL) {
        while (exam.phead->pNext != NULL) {
            tempQuestion = exam.phead;
            exam.phead = tempQuestion->pNext;
            for (i = 0; i < tempQuestion->numAnswers; i++) {
                free(tempQuestion->answers[i]->content);
                free(tempQuestion->answers[i]);
            }
            free(tempQuestion->pNext);
            free(tempQuestion->text);
            free(tempQuestion);
        }
        free(exam.phead);
    }
}

How would I free() tempQuestion in my addQuestion() function so that it only frees the memory when execution ends? I'm using Visual Studio C++ 2012 but I have to write using only C syntax (no C++). I am fairly new to C programming as well. Thanks!

Thomas Dickey
  • 51,086
  • 7
  • 70
  • 105
Benjamin
  • 345
  • 2
  • 3
  • 15
  • 2
    Please note that technically `fflush(stdin)` is undefined, it's allowed by some implementations as an extension, but it's explicitly undefined behavior in the C specification. – Some programmer dude Sep 10 '14 at 13:03
  • 1
    Also, in C [you should not cast the return of `malloc`](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). – Some programmer dude Sep 10 '14 at 13:03
  • With "C syntax", you technically don't want to cast `malloc`, but you *must* if compiling C++. – crashmstr Sep 10 '14 at 13:03
  • 1
    And don't use `gets`, it has been long deprecated and have been removed in the latest C specification. Use e.g. [`fgets`](http://en.cppreference.com/w/c/io/fgets) instead. – Some programmer dude Sep 10 '14 at 13:04
  • There are also some places in your code where you don't need pointers, for example `temQuestion->answers` could be a single pointer to `Answer` then you could use it like an array of structures instead of an array of pointers. – Some programmer dude Sep 10 '14 at 13:06
  • @JoachimPileborg It has to be an array of pointers. – Benjamin Sep 10 '14 at 13:07
  • @SJuan76 So what you are saying is move `tempExam->pNext = exam.phead` inside the for loop? – Benjamin Sep 10 '14 at 13:13

3 Answers3

1

To answer your question: You don't. You should not call free(tempQuestion) in the AddQuestion function, because that free the memory you just allocated.

When you assign a pointer to another pointer, it's only the pointer that is copied, not what it points to.

Some programmer dude
  • 400,186
  • 35
  • 402
  • 621
  • That is what I thought, but I was unsure. Is there a way then that I could release the memory when execution ends (i.e. inside CleanUp())? – Benjamin Sep 10 '14 at 13:12
  • @BenjaminC.Huskisson-Snider: Why bother freeing memory at all when the program exits? Does your platform not free all the programs resources then? – Deduplicator Sep 10 '14 at 13:21
  • @BenjaminC.Huskisson-Snider It looks to me like you're already doing it. But you also do call `free` on pointers you shouldn't, like `tempQuestion->pNext` (this causes [*undefined behavior*](http://en.wikipedia.org/wiki/Undefined_behavior) in the next iteration of the loop when you dereference the now unallocated pointer). On the other hand, you forget to free `tempQuestion->answers`. – Some programmer dude Sep 10 '14 at 13:28
  • @JoachimPileborg: The now *indeterminate* (or dangling, but certainly not unallocated) pointer. I really hope the auto-storage-class variable is still allocated, or the implementation has a huge bug. – Deduplicator Sep 10 '14 at 13:31
0

Allocate and deallocate memory for tempQuestion in main instead of allocating in AddQuestion functions.

int main ()
{
    Question *tempQuestion = malloc (sizeof (Question));
    .
    .
    .
    free (tempQuestion);
    return 0;
}

This will help you to deallocate the memory for tempQuestion just before you return from main.

Also while deallocating memory for tempQuestion->answer you missed out this

for (i = 0; i < tempQuestion->numAnswers; i++) {
    free(tempQuestion->answers[i]->content);
    free(tempQuestion->answers[i]);
}
free (tempQuestion->answer);

Rest should be ok.

Adarsh
  • 883
  • 7
  • 18
  • Please read: https://stackoverflow.com/questions/25766121/how-would-i-free-memory-allocated-to-a-pointer-in-c#comment40292911_25766121 – Deduplicator Sep 10 '14 at 13:19
  • Casting the return-value of `malloc` is at best bad style, and an unneccessary source of errors. And passing a type to `sizeof` instead of the dereferenced target-pointer is a bit more error-prone than neccessary. Read the [Q&A to casting the return-value of `malloc`](http://stackoverflow.com/q/605845) – Deduplicator Sep 10 '14 at 13:30
  • Thanks @Deduplicator for letting me know about casting return-value of malloc. – Adarsh Sep 10 '14 at 13:34
  • Here is an interesting thing. How do allocate memory for *tempQuestion* without using sizeof operator @Deduplicator. – Adarsh Sep 10 '14 at 13:35
  • Hm, I don't think I said not to use `sizeof`. Anyway, use the `sizeof`-operator thus: `T* temp = malloc(sizeof*temp);` (for any type `T`). – Deduplicator Sep 10 '14 at 13:40
  • How would you allocate memory that way, as the structure Question has lot many pointers as the member. Firstly you have to allocate memory to *testQuestion* right, once you done allocating memory to it, then you can probably use it. – Adarsh Sep 10 '14 at 13:44
  • Be aware that, unless you ask about a VLA, `sizeof` returns a compile-time-constant and the argument is unevaluated. – Deduplicator Sep 10 '14 at 13:52
0

At the end of AddQuestion() tempQuestion points to the same address that exam.phead points to, so CleanUp() frees that memory for you.

JoKo
  • 152
  • 1
  • 8