5 ms·

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

• 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.
• boomlinde 3y ago
  In your example, there are two different places where you potentially destroy the intstack. It's not particularly bad because there are only three possible resource allocation failures in your example, but it still illustrates a problem that will be worse when applying the approach as a general strategy. Consider a case where regex_canonical_equivalent_with_structures needed two stacks and two maps: intstack_t stack_a = intstack_create(256); if (!intstack_valid(stack_a)) { return regex_compare_out_of_memory; } intstack_t stack_b = intstack_create(256); if (!intstack_valid(stack_b)) { intstack_destroy(stack_a); return regex_compare_out_of_memory; } intmap_t map_a = intmap_create(16); if (!intmap_valid(map_a)) { intstack_destroy(stack_a); intstack_destroy(stack_b); return regex_compare_out_of_memory; } intmap_t map_b = intmap_create(16); if (!intmap_valid(map_b)) { intstack_destroy(stack_a); intstack_destroy(stack_b); intmap_destroy(map_a); return regex_compare_out_of_memory; } regex_compare_t res = regex_canonical_equivalent_with_structures(cache, x, y, &stack_a, &stack_b &map_a, &map_b); intstack_destroy(stack_a); intstack_destroy(stack_b); intmap_destroy(map_a); intmap_destroy(map_b); return res; } You now have a 5-tuple maintenance problem and the code isn't particularly readable. I call it a "wet staircase"; it's not DRY, it's easy to slip and it gets taller for each step :) Consider the equivalent using a simple goto escape hatch: 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_a = intstack_create(256); if (!intstack_valid(stack_a)) goto out_of_memory; intstack_t stack_b = intstack_create(256); if (!intstack_valid(stack_b)) goto out_of_memory; intmap_t map_a = intmap_create(16); if (!intmap_valid(map_a)) goto out_of_memory; intmap_t map_b = intmap_create(16); if (!intmap_valid(map_b)) goto out_of_memory; return regex_canonical_equivalent_with_structures(cache, x, y, &stack_a, &stack_b &map_a, &map_b); out_of_memory: if (intstack_valid(stack_a)) intstack_destroy(stack_a); if (intstack_valid(stack_b)) intstack_destroy(stack_b); if (intmap_valid(map_a)) intmap_destroy(map_a); if (intmap_valid(map_b)) intmap_destroy(map_b); return regex_out_of_memory; } Or if you add "destroyvalid" methods to your data structures, out_of_memory: intstack_destroyvalid(stack_a); intstack_destroyvalid(stack_b); intmap_destroyvalid(map_a); intmap_destroyvalid(map_b); return regex_out_of_memory; }