Thread: Can I get some feedback on a library I wrote for creating dynamic arrays?

I recommend that instead of writing your multi-line macros like this:

Code:

#define list_add(list, value) {typeof(value) VAL = value; list_add_ptr(list, &VAL);}

Write them like this:

Code:

#define list_add(list, value) do {typeof(value) VAL = value; list_add_ptr(list, &VAL);} while (0)

See What should be done with macros that have multiple lines? for an explanation as to why this is a better approach.

From what I see you are very close to using the opaque pointer idiom, which would give you the freedom such that if you later decide to change the layout of struct _list, programmer using your library only need to recompile your library for their programs, not all their code that uses your library by including its list.h header. What you should do is to only forward declare struct _list in list.h, then use that forward declaration to typedef ptr_list. The complete declaration (called a definition in C++) of struct _list can then be done in list.c or an internal header that is not included in list.h or any other "public" header. Likewise, typedef _list in list.c since it is never used by the interface provided to the user of the library. (Actually, you always seem to use ptr_list rather than _list, so maybe consider removing the typedef for _list entirely.)

It looks like you have a number of functions in list.c that are implementation detail, e.g., _list_index_ptr. Consider declaring them static since that will give them internal linkage, avoiding any name collisions. Speaking of _list_index_ptr, its implementation looks suspect to me:

Code:

void* _list_index_ptr(ptr_list this_list, int index)
{
    return ((void**)(this_list->_heap_ptr + (this_list->_item_size * index)));
}

My guess is that you want to add (this_list->_item_size * index) bytes to this_list->_heap_ptr, but _heap_ptr is a void* so you cannot do pointer arithmetic with it... or maybe you can because your compiler allows it, but that is not standard C. Furthermore, I don't understand why you would cast to void** only to have the return value implicitly converted to void* since that is the return type. Perhaps you should have written:

Code:

void* _list_index_ptr(ptr_list this_list, int index)
{
    return (unsigned char*)this_list->_heap_ptr + (this_list->_item_size * index);
}

It sounds like you have picked "Urbanize" as the name of your library. Considering that names like list_add are sufficiently vague so as to see use in the programs written by your potential users, and because C does not have namespace functionality, I recommend that you use a name prefix for all your library function names, e.g., urbanize_list_add.

A couple of points.

1. You should mention that you need gcc extensions to compile this.
Using the GNU Compiler Collection (GCC): Statement Exprs
Using the GNU Compiler Collection (GCC): Typeof

2. #ifndef CDYNAMICARRAYS_TEST_LIST_OPTIONS_H
Include guard names should be named after the file they're guarding.
Perhaps if everything had an 'urbanize' prefix, that would make sense.

3. You should fix all the symbols that begin with an underscore. These are reserved.

Originally Posted by c99

7.1.3 Reserved identifiers
All identifiers that begin with an underscore are always reserved for use as identifiers
with file scope in both the ordinary and tag name spaces.

4. Your use of memcmp() to compare elements (eg, list_index_of_ptr) will fail when you attempt to store structures (and even more so if you attempt to store unions). Structures can have padding which is always in an unspecified state. This can easily result in two structure instances being logically equal, but bitwise different.

5. The same goes for pointers. If I do
p = malloc(6);strcpy(p,"hello");list_add(basic_list,p) ;
I might expect
list_index_of_ptr(basic_list,"hello");
to indicate a match.