patterncMinor
Generic variable-size array
Viewed 0 times
arraygenericvariablesize
Problem
As stated in the tags, I'm a beginner to C. This code implements a dynamic array (
list.h
```
#ifndef _game_structures_list_h
#define _game_structures_list_h
#include
typedef enum {
LIST_ERR_NONE = 0x00,
LIST_ERR_POP_EMPTY = 0x01,
LIST_ERR_OUT_OF_BOUNDS = 0x02,
LIST_ERR_MEMORY = 0x03,
LIST_ERR_UNITIALIZED = 0xff
} ListError;
typedef struct {
int capacity;
int length;
ListError error;
void **data;
} List;
/**
* Creates a new List with the given starting capacity.
* @param capacity The initial allocation size for this list, not its length!
* @return A new empty List
*/
List list_new(int capacity);
/**
* Frees memory used by the list and makes it unusable
*/
void list_destroy(List *list);
/**
* Gets the error of the last operation on this list
* @return The last error, or LIST_ERR_NONE if successful
*/
ListError list_error(List *list);
/**
* Gets the amount of items in the list
* @return Length of list
*/
int list_length(List *list);
/**
* Removes all elements from the list
* @param list The list to be cleared
*/
void list_clear(List *list);
/**
* Access the item at the given index.
* @return Pointer to the item at the given index
*/
void list_get(List list, int index);
/**
* Sets the value at the given index to the item given
*/
void list_set(List list, void item, int index);
/**
* Adds an item to the end of the list
* @param list Where to add the item to
* @param item The value to be added
*/
void list_push(List list, void item);
/**
* Removes the last item from the list and returns it
* @param list The list to be popped
* @return The value removed
*/
void list_pop(List list);
/**
* Adds a new item to a list at the given position, moving all remaining items to the right
* @param list The list to which the item will be added
* @param item The val
List in .NET, vector in C++, etc) for any pointer type. I would like that it be reviewed on memory usage and code structure/style.list.h
```
#ifndef _game_structures_list_h
#define _game_structures_list_h
#include
typedef enum {
LIST_ERR_NONE = 0x00,
LIST_ERR_POP_EMPTY = 0x01,
LIST_ERR_OUT_OF_BOUNDS = 0x02,
LIST_ERR_MEMORY = 0x03,
LIST_ERR_UNITIALIZED = 0xff
} ListError;
typedef struct {
int capacity;
int length;
ListError error;
void **data;
} List;
/**
* Creates a new List with the given starting capacity.
* @param capacity The initial allocation size for this list, not its length!
* @return A new empty List
*/
List list_new(int capacity);
/**
* Frees memory used by the list and makes it unusable
*/
void list_destroy(List *list);
/**
* Gets the error of the last operation on this list
* @return The last error, or LIST_ERR_NONE if successful
*/
ListError list_error(List *list);
/**
* Gets the amount of items in the list
* @return Length of list
*/
int list_length(List *list);
/**
* Removes all elements from the list
* @param list The list to be cleared
*/
void list_clear(List *list);
/**
* Access the item at the given index.
* @return Pointer to the item at the given index
*/
void list_get(List list, int index);
/**
* Sets the value at the given index to the item given
*/
void list_set(List list, void item, int index);
/**
* Adds an item to the end of the list
* @param list Where to add the item to
* @param item The value to be added
*/
void list_push(List list, void item);
/**
* Removes the last item from the list and returns it
* @param list The list to be popped
* @return The value removed
*/
void list_pop(List list);
/**
* Adds a new item to a list at the given position, moving all remaining items to the right
* @param list The list to which the item will be added
* @param item The val
Solution
Better Naming Conventions
One of the things that immediately jumps out at me is the name you used for the data structure. As you mentioned, a dynamic array is called a
Use Opaque Types
One issue with the way you have your structure defined in the header file is that it exposes it's internal representation to the user. That is, if the user includes your header file, they are free to change the contents of the members without ever using your functions. As you can probably imagine, without proper documentation on how to properly manipulate the fields (or even with proper documentation), this is very error-prone. Ideally, you'd like your users to only be able to manipulate your structure using your API. To do this, we need to forward declare the structure in the header file and supply its definition in the implementation file. One disadvantage with this approach though is that you'll be forced to use heap allocation to initialize opaque structures.
cvector.h
cvector.c
CVector API
Having is_empty() convenience function, a function to return the current capacity of the vector, a
Avoid Macros When They Are Unnecessary
In your case, all the macros are completely unnecessary and could just be implemented as either small
You gain the benefits of type safety and self-documenting code with this.
EDIT:
Revise allocation strategy
Currently, your allocation strategy is forcing you to reallocate every single time a modifying operation is done on a vector. As an example, assume I create a vector with 10 as the initial capacity:
Then I push an element into the vector. Your code would check and see that my
One of the things that immediately jumps out at me is the name you used for the data structure. As you mentioned, a dynamic array is called a
List in C# and in Java. However, in the C/C++ world, we generally like to refer to dynamic arrays as either (ahem) dynamic arrays or vectors. When I saw List, my brain automatically started thinking I was going to be looking at code for a singly/doubly linked list of some sort (since I come from a C/C++ world). I'm sure others would find the use of the name List to describe a vector a little misleading as well. Obviously, you'd need to rename all your functions and your error enumeration to reflect this name change and keep the API consistent. For the code I'll be presenting, I'll be calling the structure a cvector.Use Opaque Types
One issue with the way you have your structure defined in the header file is that it exposes it's internal representation to the user. That is, if the user includes your header file, they are free to change the contents of the members without ever using your functions. As you can probably imagine, without proper documentation on how to properly manipulate the fields (or even with proper documentation), this is very error-prone. Ideally, you'd like your users to only be able to manipulate your structure using your API. To do this, we need to forward declare the structure in the header file and supply its definition in the implementation file. One disadvantage with this approach though is that you'll be forced to use heap allocation to initialize opaque structures.
cvector.h
// forward declare here
typedef struct cvector_ cvector;
// no need to assign values to enumerations past the first one since
// the compiler automatically provides incremented values for them.
typedef enum {
CVECTOR_ERR_NONE = 0x00,
CVECTOR_ERR_POP_EMPTY,
CVECTOR_ERR_OUT_OF_BOUNDS,
CVECTOR_ERR_MEMORY,
CVECTOR_INVALID_OPERATION,
CVECTOR_ERR_UNITIALIZED
} CVectorError;cvector.c
// define structure in here
struct cvector_ {
// these values can never be negative so use unsigned numbers to reflect that
size_t capacity, size;
void **data;
};CVector API
Having is_empty() convenience function, a function to return the current capacity of the vector, a
reserve() function to reserve memory storage, and maybe a shrink_to_fit() function would be great to have. You will also want to have init() and free() functions to both initialize and destroy the vector you created. Also, notice that I removed the error code member from the vector structure. It isn't needed because if you design your API to return error codes where appropriate, the user could just query the return result of whatever API function they're calling. Moreover, it serves to just bloat the data structure unnecessarily. You should look to have your set(), get(), push(), pop(), insert(), and remove() functions return error codes that the user could query if they so choose. Below is a simplified implementation of some of the functions I talked about to give you an idea of what I mean.cvector *cvector_init(size_t capacity)
{
cvector *cv = calloc(1, sizeof(*cv));
if (cv && internal_cvector_reserve(cv, capacity) != CVECTOR_ERR_NONE) {
free(cv);
cv = NULL;
}
return cv;
}
void cvector_free(cvector *cv)
{
free(cv->data);
free(cv);
}
static CVectorError internal_cvector_reserve(cvector *cv, size_t new_capacity)
{
void **new_data = realloc(cv->data, new_capacity * sizeof(*(cv->data)));
if (!new_data) {
return CVECTOR_ERR_MEMORY;
}
cv->data = new_data;
cv->capacity = new_capacity;
return CVECTOR_ERR_NONE;
}
CVectorError cvector_reserve(cvector *cv, size_t new_capacity)
{
return new_capacity > cv->capacity ?
internal_cvector_reserve(cv, new_capacity) :
CVECTOR_INVALID_OPERATION;
}
bool cvector_empty(const cvector *cv)
{
return cvector->size == 0;
}
CVectorError cvector_shrink_to_fit(cvector *cv)
{
return internal_cvector_reserve(cv, cv->size);
}Avoid Macros When They Are Unnecessary
In your case, all the macros are completely unnecessary and could just be implemented as either small
static functions or as typed constants. This one is especially egregious: L_ERR . That's because it hides a return statement and makes reading the code using it very very awkward. Instead of the CAPACITY_FACTOR macro, you could just as easily use:static const size_t CAPACITY_FACTOR = 2;You gain the benefits of type safety and self-documenting code with this.
EDIT:
Revise allocation strategy
Currently, your allocation strategy is forcing you to reallocate every single time a modifying operation is done on a vector. As an example, assume I create a vector with 10 as the initial capacity:
Capacity: 10
Size: 0Then I push an element into the vector. Your code would check and see that my
Code Snippets
// forward declare here
typedef struct cvector_ cvector;
// no need to assign values to enumerations past the first one since
// the compiler automatically provides incremented values for them.
typedef enum {
CVECTOR_ERR_NONE = 0x00,
CVECTOR_ERR_POP_EMPTY,
CVECTOR_ERR_OUT_OF_BOUNDS,
CVECTOR_ERR_MEMORY,
CVECTOR_INVALID_OPERATION,
CVECTOR_ERR_UNITIALIZED
} CVectorError;// define structure in here
struct cvector_ {
// these values can never be negative so use unsigned numbers to reflect that
size_t capacity, size;
void **data;
};cvector *cvector_init(size_t capacity)
{
cvector *cv = calloc(1, sizeof(*cv));
if (cv && internal_cvector_reserve(cv, capacity) != CVECTOR_ERR_NONE) {
free(cv);
cv = NULL;
}
return cv;
}
void cvector_free(cvector *cv)
{
free(cv->data);
free(cv);
}
static CVectorError internal_cvector_reserve(cvector *cv, size_t new_capacity)
{
void **new_data = realloc(cv->data, new_capacity * sizeof(*(cv->data)));
if (!new_data) {
return CVECTOR_ERR_MEMORY;
}
cv->data = new_data;
cv->capacity = new_capacity;
return CVECTOR_ERR_NONE;
}
CVectorError cvector_reserve(cvector *cv, size_t new_capacity)
{
return new_capacity > cv->capacity ?
internal_cvector_reserve(cv, new_capacity) :
CVECTOR_INVALID_OPERATION;
}
bool cvector_empty(const cvector *cv)
{
return cvector->size == 0;
}
CVectorError cvector_shrink_to_fit(cvector *cv)
{
return internal_cvector_reserve(cv, cv->size);
}static const size_t CAPACITY_FACTOR = 2;Capacity: 10
Size: 0Context
StackExchange Code Review Q#125891, answer score: 2
Revisions (0)
No revisions yet.