I am developing a very basic CLI flag utility library in C. The following code is a part of the main translation unit containing all the relevant functions, and is located in the global scope.
struct pair {
char name[100];
char defaultValue[100];
};
static struct pair *defaults = NULL;
void setDefault(const char *flagName, const char *defaultValue) {
if (defaults == NULL) {
defaults = malloc(10 * sizeof(struct pair));
}
static short pos = 0;
if(pos % 10 == 0) {
defaults = realloc(defaults, (pos + 10) * sizeof(struct pair));
}
strncpy((defaults + pos)->name, flagName, 100);
strncpy((defaults + pos)->defaultValue, defaultValue, 100);
pos++;
}
static void clearDefaults(void) {
free(defaults);
}
To clue you in, the idea is that the user calls setDefault
with 2 strings, the first of which represents a flag name, and the second which represents the default value of that (should it not have been passed to the program). The pair is stored in memory and is later used by other library functions.
My question is centered around the storage of flagName:defaultValue
pairs, which are represented using the pair
struct. Right now I'm using a global static variable defaults
which stores a pointer to a dynamically allocated array of said struct
s. It's static, therefore I had to initially set it to NULL, and "initialize" it only after the first call of setDefaults
. I feel that this creates unnecessary clutter as other library functions have to test defaults
for NULL
before trying to search it for a specific pair.
That is my first concern, is there a better way to initialize a global static variable?
The second is my implementation of dynamic reallocation in setDefault
. It initially allocates space for 10 pairs, and once the limit is reached it allocated space for 10 more and so on. I've tested the implementation with Valgrind and a bunch of calls to setDefault
and there were no memory leaks or other errors.
Is this a good way to do it? Can anything be improved? I'm very much a beginner at C so any feedback is welcome.
1 Answer 1
Yes, there are things to improve:
if (defaults == NULL) {
defaults = malloc(10 * sizeof(struct pair));
}
static short pos = 0;
if(pos % 10 == 0) {
defaults = realloc(defaults, (pos + 10) * sizeof(struct pair));
}
That allocates space for 10 elements, and then re-allocates space for ten elements. What a waste of time.
Additionally, avoid sizeof(TYPE)
. sizeof expr
is safer, and will be obviously correct.
Next, allocating memory is known to fail sometimes. So, check for that.
And finally, rework setDefault()
and clearDefaults()
so the latter actually only clears the defaults.
bool setDefault(const char *flagName, const char *defaultValue) {
static short pos = 0;
if (!defaults) pos = 0;
if (pos % 10 == 0) {
struct pair* temp = realloc(defaults, (pos + 10) * sizeof *temp);
if (!temp) return false;
defaults = temp;
}
...
}
static void clearDefaults(void) {
free(defaults);
defaults = NULL;
}
Are you sure you want to use strncpy()
? You know that's not really a string-function, but a zero-padding function?
Another interesting question is how the other functions should know how many elements are in the list. I have no idea, design something.
Also, consider being more dynamic, and just saving two pointers to independently-allocated strings in the pair-element.
And if having defaults
starts as a null-pointer-value really disturbs you, just initialize it to oint to a statically-allocated null-object.
-
\$\begingroup\$ Thank you! What do you mean by
strncpy
not being a real string function? I understand that it right-pads the source string with null bytes, but why is that a problem? What should I use instead? Thanks for all the other tips. \$\endgroup\$bool3max– bool3max2018年08月29日 12:38:06 +00:00Commented Aug 29, 2018 at 12:38 -
2\$\begingroup\$ What I mean is 1) as you said, it 0-pads the target, 2) it does not 0-terminate the target, and 3) it does not require the source to be a string. Of course, if the source has a 0 early enough, the 0-padding will act as 0-termination too. \$\endgroup\$Deduplicator– Deduplicator2018年08月29日 12:39:45 +00:00Commented Aug 29, 2018 at 12:39
-
\$\begingroup\$ So plain
strcpy
would be a better alternative here? \$\endgroup\$bool3max– bool3max2018年08月29日 13:13:44 +00:00Commented Aug 29, 2018 at 13:13 -
1\$\begingroup\$ To well answer what a better alternative is for
strcnpy()
, please clearly answer this first: Should the string pointed to byflagName
not fit in(defaults + pos)->name
, what functionality do you want? \$\endgroup\$chux– chux2018年08月29日 13:35:06 +00:00Commented Aug 29, 2018 at 13:35
getopt
and its clones? \$\endgroup\$