3 ms·
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
by 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 2y agoIn 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 2y agoUnfortunately your version doesn't free map and stack in case of success.
- JonChesterfield 2y agoIt'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.