[unwind]

Okay, I love C so of course I had to take a look. The buzzwords are impressive, with all the testing and CI and so on, really nice, modern and ambitious.

I dove into the code, literally looking at the first true part of the implementation in array/sc_array.h.

Two observations, from probably less than tree minutes of reading:

1. The nomenclature with "sc_array_term()" as the destructor (the opposite of sc_array_init()) was new to me; I'm not saying that's a real problem but it's at least adding friction in a way. I would have expected sc_array_free(), but that seems to be internally used as an alias for the standard function free(). Very confusing, since that name (sc_array_free()) then leaks into the source but it has the feeling of being meant for internal use.

2. I wonder if this has a bug:

    /**
     * Deletes items from the array without deallocating underlying memory
     * @param a array
     */
    #define sc_array_clear(a) \
    do {                                                                   \
      (a)->cap = 0;                                                  \
      (a)->size = 0;                                                 \
      (a)->oom = false;                                              \
    } while (0)

In my experience, when you want to clear a dynamic array but keep the allocated memory, the 'capacity' ('cap', here) field should not be cleared. I think this will leak memory if an array is grown, cleared, and then re-filled since the sc_array_add() function will see a zero capacity, and allocate new storage.

Just my inflation-devalued SEK 0.02, and I did not run the code, I just read it very quickly. Corrections welcome.

[j1elo]

> 1. The nomenclature with "sc_array_term()" as the destructor (the opposite of sc_array_init()) was new to me; I'm not saying that's a real problem but it's at least adding friction in a way.

I think there shouldn't be any need to innovate in the naming of these kinds of functions... GLib got it perfectly right [0] with the pair init / finalize. One might like the alternative init / deinit if the goal is to have some symmetry in the naming... but there are not many better choices.

free is to be used (always IMHO) typically as the opposite of new, make, or create.

"Naming things is difficult" and all that...

[0]: https://docs.gtk.org/gobject/concepts.html

[zoomablemind]

>... /* * Deletes items from the array without deallocating underlying memory * @param a array */

This does look like a bug, if not functionally, then by the described intent. The comment states that the intent is to delete items without deallocating; yet zeroing the capacity cuts any future way to deallocate properly, unless it's just an array of simple types (free(baseptr)). If there are any pointer types, then there will remain no safe way to deallocate the elements' contents.

Sure, there could be other ways of keeping inventory of the allocated memory, but this detaches the array from knowing its allocated bounds.

[antirez]

The macro does not look (necessarily) as a bug, it simply does a counter intuitive thing of zeroing everything, including the capacity. Maybe the way it is used, the capacity is saved, then the array cleared, then it is set back; or more likely it is used only after the allocation of the object, when everything requires to be zeroed (but if this is the case it should be called "_init" and not "_clear", for clarity). Does not look as the most sounding interface of course. Also this is the kind of thing that should not be done as macro regardless of speed... and only turned into a macro in case of very aggressive profiler-drive optimization.

[unwind]

If the way it is used requires the user to break the abstraction/encapsulation and manually buffer some fields in order not to break the data structure and leak memory, I would call that a bug.

There is one use of sc_array_clear() in the test code [1] which really makes it look as if it is being used in a way that I think (again, I haven't single-stepped this code, only read it) leaks memory.

I agree on the pain of everything being macros, it's more pain than it's worth I think and will likely lead to code duplication (and more pain in debugging, probably).

I would even go so far as to think that this kind of single-file design, where each file is independent of the others, makes it harder and more annoying to implement more complicated data structures.

[1]: https://github.com/tezc/sc/blob/master/array/array_test.c#L3...

[lelanthran]

I'm too lazy to look at the code that does the alloc, but if this came my way in a PR I'd ask if the code doing the allocation is using malloc or realloc.

If all allocations are performed using realloc[1] then I have no problem with that macro.

[1] Sometimes it's just easier. Why conditionally call malloc/realloc when you can simply call realloc all the time? Realloc(NULL, size) is equivalent to malloc(size).

[me_me_me]

> it simply does a counter intuitive thing of zeroing everything

It might not be an implementation bug, but that is just asking for bugs. Possibly very hard to find bugs.

[l-albertovich]

Glanced over it, it doesn't look like a bug, just wasteful.

The way I see it, because sc_array_clear resets cap and size to zero, the next call to sc_array_add will issue a realloc call to the default minimum size (8) and it won't leak because sc_array_clear doesn't overwrite elems.

So yeah, not cool but doesn't look like a bug either.

[danielEM]

I see just one GH issue and it is none of yours above, why?

As for someone claiming so much appreciation for C it seem like your love is a bit toxic ;-) (no offense here).