4 ms·

Good "Go To Statement Considered Harmful" considered harmful take. I frequently use goto in C to jump to resource cleanup code in case of an error. Always turns

Good "Go To Statement Considered Harmful" considered harmful take. I frequently use goto in C to jump to resource cleanup code in case of an error. Always turns out more readable than the alternatives. I think I may even have used it in Go once, but I can't remember what for. Go's defer keyword obviates the above use of goto, and labeled breaks almost any other case I can think of.

buescher 3y ago
I wouldn't die on that hill but I agree and have done the same. In fact I would argue that if you find yourself in arguments about the practice it is a good sign your team is at a level where you should in fact prohibit all use of goto. IIRC at least one of the revisions of MISRA specifically allows for using goto to break forward to a cleanup-and-return block, but others don't.
JonChesterfield 3y ago
You might find splitting the functions into two works well. The outer one tries to allocate whatever resources are needed, the inner one assumes they have been allocated. Ends up looking something like (copied from one of my C codebases) static regex_compare_t regex_canonical_equivalent_with_structures( regex_cache_t *cache, stringtable_index_t x, stringtable_index_t y, intstack_t *stack_arg, intmap_t *map_arg) { // fairly complicated code using those structures } regex_compare_t regex_canonical_equivalent(regex_cache_t *cache, stringtable_index_t x, stringtable_index_t y) { if (x.value == y.value) { return regex_compare_equal; } intstack_t stack = intstack_create(256); if (!intstack_valid(stack)) { return regex_compare_out_of_memory; } intmap_t map = intmap_create(16); if (!intmap_valid(map)) { intstack_destroy(stack); return regex_compare_out_of_memory; } regex_compare_t res = regex_canonical_equivalent_with_structures(cache, x, y, &stack, &map); intstack_destroy(stack); intmap_destroy(map); return res; }
- teo_zero 3y ago
  Yours seems a good practice. But apart from having concentrated it in the outer function, you have not reduced the cleanup burden. In fact, the outer function might grow out of control if the number of resources increases. In your example, should you allocate foo and bar after stack and map, you need to remember to add the respective destroy's to all failure path, and possibly in the correct reversed order (which you don't show in your code for map & stack, by the way). You'll soon find yourself with 4 identical calls to intstack_destroy() to maintain. Instead, I'll advocate for a cleanup section at the end similar to cleanup: if (bar) bar_destroy(bar); if (foo) foo_destroy(foo); if (map) intmap_destroy(map); if (stack) intstack_destroy(stack); return res; And I would replace all the if (!something) with, e.g. if (!map) { res = regex_out_of_memory; goto cleanup; }
  - boomlinde 3y ago
    In this case specifically, because the allocation failures both have the same result code, I would do the following: regex_compare_t regex_canonical_equivalent(regex_cache_t *cache, stringtable_index_t x, stringtable_index_t y) { if (x.value == y.value) { return regex_compare_equal; } intstack_t stack = intstack_create(256); if (!intstack_valid(stack)) goto out_of_memory; intmap_t map = intmap_create(16); if (!intmap_valid(map)) goto out_of_memory; return regex_canonical_equivalent_with_structures(cache, x, y, &stack, &map); out_of_memory: if (intstack_valid(stack)) intstack_destroy(stack); if (intmap_valid(map)) intmap_destroy(map); return regex_out_of_memory; } Better yet if your data structures have "destroyvalid" methods.
    - teo_zero 3y ago
      Unfortunately your version doesn't free map and stack in case of success.
    - JonChesterfield 3y ago
      It's pretty. I like the tail call. Sadly jumping over the initialisation of map then calling a function on it can't work, plus the leak. Maybe introducing cannot-fail default constructors is the way to go, allowing something like: intstack_t stack = intstack_default(); intmap_t map = intmap_default(); regex_compare_t res = regex_out_of_memory; stack = intstack_create(256); if (!intstack_valid(stack)) goto out_of_memory; map = intmap_create(16); if (!intmap_valid(map)) goto out_of_memory; res = regex_canonical_equivalent_with_structures(cache, x, y, &stack, &map); out_of_memory: intstack_destroy(stack); intmap_destroy(map); return res; It should be possible to structure things so one doesn't accumulate stack frames and also keeps the resource logic separate from the implementation but it's not immediately obvious to me what a pretty way to do that is.

buescher 3y ago

I wouldn't die on that hill but I agree and have done the same. In fact I would argue that if you find yourself in arguments about the practice it is a good sign your team is at a level where you should in fact prohibit all use of goto. IIRC at least one of the revisions of MISRA specifically allows for using goto to break forward to a cleanup-and-return block, but others don't.

JonChesterfield 3y ago

You might find splitting the functions into two works well. The outer one tries to allocate whatever resources are needed, the inner one assumes they have been allocated. Ends up looking something like (copied from one of my C codebases) static regex_compare_t regex_canonical_equivalent_with_structures( regex_cache_t *cache, stringtable_index_t x, stringtable_index_t y, intstack_t *stack_arg, intmap_t *map_arg) { // fairly complicated code using those structures } regex_compare_t regex_canonical_equivalent(regex_cache_t *cache, stringtable_index_t x, stringtable_index_t y) { if (x.value == y.value) { return regex_compare_equal; } intstack_t stack = intstack_create(256); if (!intstack_valid(stack)) { return regex_compare_out_of_memory; } intmap_t map = intmap_create(16); if (!intmap_valid(map)) { intstack_destroy(stack); return regex_compare_out_of_memory; } regex_compare_t res = regex_canonical_equivalent_with_structures(cache, x, y, &stack, &map); intstack_destroy(stack); intmap_destroy(map); return res; }

teo_zero 3y ago
Yours seems a good practice. But apart from having concentrated it in the outer function, you have not reduced the cleanup burden. In fact, the outer function might grow out of control if the number of resources increases. In your example, should you allocate foo and bar after stack and map, you need to remember to add the respective destroy's to all failure path, and possibly in the correct reversed order (which you don't show in your code for map & stack, by the way). You'll soon find yourself with 4 identical calls to intstack_destroy() to maintain. Instead, I'll advocate for a cleanup section at the end similar to cleanup: if (bar) bar_destroy(bar); if (foo) foo_destroy(foo); if (map) intmap_destroy(map); if (stack) intstack_destroy(stack); return res; And I would replace all the if (!something) with, e.g. if (!map) { res = regex_out_of_memory; goto cleanup; }
- boomlinde 3y ago
  In this case specifically, because the allocation failures both have the same result code, I would do the following: regex_compare_t regex_canonical_equivalent(regex_cache_t *cache, stringtable_index_t x, stringtable_index_t y) { if (x.value == y.value) { return regex_compare_equal; } intstack_t stack = intstack_create(256); if (!intstack_valid(stack)) goto out_of_memory; intmap_t map = intmap_create(16); if (!intmap_valid(map)) goto out_of_memory; return regex_canonical_equivalent_with_structures(cache, x, y, &stack, &map); out_of_memory: if (intstack_valid(stack)) intstack_destroy(stack); if (intmap_valid(map)) intmap_destroy(map); return regex_out_of_memory; } Better yet if your data structures have "destroyvalid" methods.
  - teo_zero 3y ago
    Unfortunately your version doesn't free map and stack in case of success.
  - JonChesterfield 3y ago
    It's pretty. I like the tail call. Sadly jumping over the initialisation of map then calling a function on it can't work, plus the leak. Maybe introducing cannot-fail default constructors is the way to go, allowing something like: intstack_t stack = intstack_default(); intmap_t map = intmap_default(); regex_compare_t res = regex_out_of_memory; stack = intstack_create(256); if (!intstack_valid(stack)) goto out_of_memory; map = intmap_create(16); if (!intmap_valid(map)) goto out_of_memory; res = regex_canonical_equivalent_with_structures(cache, x, y, &stack, &map); out_of_memory: intstack_destroy(stack); intmap_destroy(map); return res; It should be possible to structure things so one doesn't accumulate stack frames and also keeps the resource logic separate from the implementation but it's not immediately obvious to me what a pretty way to do that is.