patterncMinor
Dynamic Array container in macros
Viewed 0 times
arraymacroscontainerdynamic
Problem
The code below is an attempt to mimic
The implementation is purely macros, in the C99 dialect, and requires the non-standard
I am interested to learn of both coding mistakes and design mistakes.
The project resides on SourceForge, in case example code is needed for the review process.
`//
// ctnr - Containers for the C language
//
// Written in 2016 by Andrei Bondor, ab396356@users.sourceforge.net
//
// To the extent possible under law, the author(s) have dedicated all copyright
// and related and neighboring rights to this software to the public domain
// worldwide. This software is distributed without any warranty.
//
// You should have received a copy of the CC0 Public Domain Dedication along
// with this software (as file "copying.txt"). If not, see:
//
// http://creativecommons.org/publicdomain/zero/1.0/
//
///
/// @file
/// @brief Implements the Dynamic Array data structure.
/// @details
///
/// Memory management:
/// ------------------
/// DYNARRAY_MALLOC
/// DYNARRAY_REALLOC
/// DYNARRAY_FREE
///
/// General functions:
/// ------------------
/// DynArrayType dynarray (ElemType)
/// bool dynarray_create (DynArray, Capacity)
/// void dynarray_ptrcopy (DynArray, ElemPtr, Count)
/// void dynarray_destroy (DynArray)
/// void dynarray_clear (DynArray)
/// size_t dynarray_size (DynArray)
/// void dynarray_search (DynArray, ValidFunc, DynArrayRes)
/// void dynarray_count (DynArray, ElemVal, CountRes)
/// bool dynarray_empty (DynArray)
///
/// Iterator support:
/// -----------------
/// ElemIterType dynarrayiter (DynArray)
/// ElemVal dynarrayiter_deref (ElemIter)
/// void dynarrayiter_inc (ElemIter)
/// ElemIter dynarr
std::vector from the C++ standard library in the C language.The implementation is purely macros, in the C99 dialect, and requires the non-standard
__typeof__ keyword.I am interested to learn of both coding mistakes and design mistakes.
The project resides on SourceForge, in case example code is needed for the review process.
`//
// ctnr - Containers for the C language
//
// Written in 2016 by Andrei Bondor, ab396356@users.sourceforge.net
//
// To the extent possible under law, the author(s) have dedicated all copyright
// and related and neighboring rights to this software to the public domain
// worldwide. This software is distributed without any warranty.
//
// You should have received a copy of the CC0 Public Domain Dedication along
// with this software (as file "copying.txt"). If not, see:
//
// http://creativecommons.org/publicdomain/zero/1.0/
//
///
/// @file
/// @brief Implements the Dynamic Array data structure.
/// @details
///
/// Memory management:
/// ------------------
/// DYNARRAY_MALLOC
/// DYNARRAY_REALLOC
/// DYNARRAY_FREE
///
/// General functions:
/// ------------------
/// DynArrayType dynarray (ElemType)
/// bool dynarray_create (DynArray, Capacity)
/// void dynarray_ptrcopy (DynArray, ElemPtr, Count)
/// void dynarray_destroy (DynArray)
/// void dynarray_clear (DynArray)
/// size_t dynarray_size (DynArray)
/// void dynarray_search (DynArray, ValidFunc, DynArrayRes)
/// void dynarray_count (DynArray, ElemVal, CountRes)
/// bool dynarray_empty (DynArray)
///
/// Iterator support:
/// -----------------
/// ElemIterType dynarrayiter (DynArray)
/// ElemVal dynarrayiter_deref (ElemIter)
/// void dynarrayiter_inc (ElemIter)
/// ElemIter dynarr
Solution
General comments
Be aware that there are other C dynamic array implementations available, including at least one other I can think of that is implemented entirely with macros. I am uncertain, however, whether there are any others that attempt to cleave so closely to the C++
Macro form
The usual convention for the form of a macro that must provide a code block is to use
(See the Linux Kernel style guide, or this one from CMU, or this one from multiple parties including Bell Labs and UC Berkeley.)
Following convention will make your code easier for others to understand and maintain. It will also avoid various compilers emitting unused-value warnings for uses of your macros, and since some projects are very particular about avoiding warnings, the warnings caused by the present form of your macros could be a deal-breaker.
Multiple evaluation
More importantly, however, most of your macros evaluate one or more of their arguments more than once. This makes them very dangerous to use, and if I were evaluating your macro set for use in a project of mine, that by itself would be enough for me to reject it. As long as you are accepting dependence on
(Note that typical implementations of
Error handling
Several of your macros afford the opportunity for memory allocation failures. You appear generally to have reasonable handling for those cases, except that there is no mechanism for informing the user that a failure has occurred. A user who delved deeply enough into your implementation could probably figure out per-macro means to check whether each macro succeeded, but the macro set ought to provide a standard mechanism. Perhaps you could provide a member in your dynamic array structures that serves as an error flag, and macros to test and reset it.
(Lack of) memory management
The code does a decent job (modulo my preceding comments) of managing the memory associated with each dynamic array itself, but it has no provision for managing memory associated with array elements. That becomes important when elements are pointers to dynamically-allocated memory. This might be acceptable, and certainly it leaves your code simpler, but it would be well to at least discuss the topic either in the general documentation or in the documentation of macros that can cause elements to be lost.
Specific Comments
Don't rely on pragmas
Other than a handful of standard ones, pragmas are implementation-specific. Do not rely on them in code you want to be portable; specifically, in this case, use standard guard macros to protect against multiple inclusion, not
This macro takes a declared dynamic array structure as an argument. It does not create a dynamic array, as its name suggests; instead, it initializes one. Personally, I would name such a macro
Since
Be aware that there are other C dynamic array implementations available, including at least one other I can think of that is implemented entirely with macros. I am uncertain, however, whether there are any others that attempt to cleave so closely to the C++
std::vector interface. I am by no means convinced that that's a desirable characteristic, but neither am I criticizing the project on that basis.Macro form
The usual convention for the form of a macro that must provide a code block is to use
do / while, not if / else:#define foo(x) do { /* do something */ } while (0)(See the Linux Kernel style guide, or this one from CMU, or this one from multiple parties including Bell Labs and UC Berkeley.)
Following convention will make your code easier for others to understand and maintain. It will also avoid various compilers emitting unused-value warnings for uses of your macros, and since some projects are very particular about avoiding warnings, the warnings caused by the present form of your macros could be a deal-breaker.
Multiple evaluation
More importantly, however, most of your macros evaluate one or more of their arguments more than once. This makes them very dangerous to use, and if I were evaluating your macro set for use in a project of mine, that by itself would be enough for me to reject it. As long as you are accepting dependence on
__typeof__, however, I think there is a workaround that will work for most of your macros, along these lines:#define dynarray_ptrcopy(DynArray, ElemPtr, Count) do { \
__typeof__(DynArray) *da = &(DynArray); \
size_t da_count = (Count); \
void *ptrdata = DYNARRAY_REALLOC( \
da->data, \
(sizeof *da->data) * da_count); \
if (ptrdata != NULL) { \
da->data = ptrdata; \
da->count = da_count; \
da->capacity = da_count; \
memcpy( \
da->data, \
(ElemPtr), \
(sizeof *da->data) * da_count); \
} \
} while (0)(Note that typical implementations of
__typeof__(), like sizeof(), do not evaluate their operands). That approach will not work for dynarray_create(), however, or any other macro that must expand to an expression, because declarations cannot appear inside expressions in C.Error handling
Several of your macros afford the opportunity for memory allocation failures. You appear generally to have reasonable handling for those cases, except that there is no mechanism for informing the user that a failure has occurred. A user who delved deeply enough into your implementation could probably figure out per-macro means to check whether each macro succeeded, but the macro set ought to provide a standard mechanism. Perhaps you could provide a member in your dynamic array structures that serves as an error flag, and macros to test and reset it.
(Lack of) memory management
The code does a decent job (modulo my preceding comments) of managing the memory associated with each dynamic array itself, but it has no provision for managing memory associated with array elements. That becomes important when elements are pointers to dynamically-allocated memory. This might be acceptable, and certainly it leaves your code simpler, but it would be well to at least discuss the topic either in the general documentation or in the documentation of macros that can cause elements to be lost.
Specific Comments
Don't rely on pragmas
Other than a handful of standard ones, pragmas are implementation-specific. Do not rely on them in code you want to be portable; specifically, in this case, use standard guard macros to protect against multiple inclusion, not
#pragma once.dynarray_create() is misleadingly namedThis macro takes a declared dynamic array structure as an argument. It does not create a dynamic array, as its name suggests; instead, it initializes one. Personally, I would name such a macro
dynarray_init().dynarray_destroy() should set the data pointer to NULLSince
dynarray_destroy() does not, in fact, destroy the dynamic array object itself, it should do its best to leave it in a consistent state. That would involve setting its data pointer to NULL after freeing it. This leaves it in the saCode Snippets
#define foo(x) do { /* do something */ } while (0)#define dynarray_ptrcopy(DynArray, ElemPtr, Count) do { \
__typeof__(DynArray) *da = &(DynArray); \
size_t da_count = (Count); \
void *ptrdata = DYNARRAY_REALLOC( \
da->data, \
(sizeof *da->data) * da_count); \
if (ptrdata != NULL) { \
da->data = ptrdata; \
da->count = da_count; \
da->capacity = da_count; \
memcpy( \
da->data, \
(ElemPtr), \
(sizeof *da->data) * da_count); \
} \
} while (0)Context
StackExchange Code Review Q#144300, answer score: 3
Revisions (0)
No revisions yet.