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

  1. #1
    Registered User
    Join Date
    May 2018

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

    Here is the link: GitHub - NateSeymour/urbanize: Fast, dynamically allocated, type independent arrays in C.

    I think it's pretty new and creative how I create and add to the dynamically allocated arrays, but you can tell me. Examples are in the Readme and in the examples folder.

    Thank you very much :)

  2. #2
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    I recommend that instead of writing your multi-line macros like this:
    #define list_add(list, value) {typeof(value) VAL = value; list_add_ptr(list, &VAL);}
    Write them like this:
    #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:
    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:
    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.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  3. #3
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    The edge of the known universe
    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

    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.
    Quote 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
    to indicate a match.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Can I get some feedback on this paper I wrote?
    By MutantJohn in forum General Discussions
    Replies: 8
    Last Post: 06-15-2015, 10:39 PM
  2. Creating Static and Dynamic Library
    By arka.sharma in forum C Programming
    Replies: 1
    Last Post: 09-03-2011, 05:42 AM
  3. Creating and freeing dynamic arrays
    By circuitbreaker in forum C++ Programming
    Replies: 8
    Last Post: 02-18-2008, 11:18 AM
  4. Creating New Dynamic Arrays
    By mas0nite in forum C++ Programming
    Replies: 8
    Last Post: 10-19-2006, 02:59 PM
  5. creating dynamic arrays
    By Cnewb in forum C Programming
    Replies: 11
    Last Post: 12-01-2002, 06:13 PM

Tags for this Thread