Hacker News Reader

Comment by bluecalm | Hacker News Reader

As mentioned above, in C goto is often useful for error handling (where you need to free some resources before exiting the function). It's way more convenient and readable to write freeing code once and jump to it instead of having multiple return exits and duplicate the code before every single one of them.

View on HN

What the parent said still applies in your case. You just make a wrapper function that does the cleanup. I have used this pattern many times:

  static int foo_impl(int arg, int **resource1, int **resource2) {
    *resource1 = (int *)malloc(sizeof(**resource1));
    if (*resource1 == NULL) return EXIT_FAILURE;
    /* Do something with resource1 (omitted)... */
    *resource2 = (int *)malloc(sizeof(**resource2));
    if (*resource2 == NULL) return EXIT_FAILURE;
    /* Do something with resource1 and resource2 (e.g.,
       stick them in a global hash table as key/value pairs,
       or whatever, omitted)... */
    return EXIT_SUCCESS;
  }

  int foo(int arg) {
    int *resource1;
    int *resource2;
    int ret;
    resource1 = resource2 = NULL;
    ret = foo_impl(arg, &resource1, &resource2);
    if (ret != EXIT_SUCCESS) {
      free(resource2);
      free(resource1);
    }
    return ret;
  }

The advantages are that it makes it very explicit which resources need to be cleaned up (making it easier to review to be sure you haven't forgotten one, or forgotten to initialize one, because they're in a nice list), and it's harder to screw up the control flow. You can't get out of the function without passing by the clean-up code, and you can't accidentally fall into the clean-up code without explicitly returning an error.

I know it is just an example to illustrate the point but this seems to have a bug in it: resource2 won't be initialized if the function fail to allocate resource1 so resource2 will be null when the code enters the deallocation condition.

A way to fix it would be to return different failures for every allocation and to use a switch without a break to clean up, something like:

    switch (ret) {
      case EXIT_SUCCESS:
        free(resource2);
      case EXIT_FAILURE2: 
        free(resource1);
      case EXIT_FAILURE1:
        break;
    }

EDIT: added potential fix to the code. Sorry if there is any bug in the fix, my C is a little bit ... rusty.

EDIT2: fixed a bug in the fix. C is hard, let's go shopping.

EDIT3: the explanation for the bug was also wrong. It is not a memory leak for but freeing a null pointer. Fixed too.

Passing a null pointer to free() is perfectly safe, and in this case simplifies the cleanup code.

Although the concern of a memory leak is valid. I would have expected the resources to be free'd regardless of the result.

Of course, you are right. I seem to have carried the belief that freeing a null pointer was as bad as freeing one twice. And now I know.

Seems to be a common misconception [1], thanks for pointing out, that makes the GGP code correct in all counts and my switch redundant.

[1] https://news.ycombinator.com/item?id=8844031

Except of course that many languages seem to be trying to remove fallthroughs from switches too...

Or you could simply free resource1 and resource2 even if the allocation failed. free(NULL) is defined to do nothing.

I find myself often having the issue where the code digging through what can and often is, garbage. At some point you find it's pointless to continue. You are done. Fin. Goto error; in that case makes absolutely perfect sense.

Two things about goto and jmp instructions. Most people have no idea how badly they were abused back in old days. For instance to jump into the middle of a subroutine. Yay just saved 9!!! words of core memory!!! And most people forget that old computer scientists were obsessed with creating grammars that you could write formal proofs for.

Reply on news.ycombinator.com