0
\$\begingroup\$

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 structs. 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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Are you deliberately reinventing POSIX getopt and its clones? \$\endgroup\$ – 200_success Aug 29 '18 at 0:30
  • \$\begingroup\$ @200_success To be honest I had no idea they existed. Though I'm doing this for fun so it's not a concern. \$\endgroup\$ – Bogdan M. Aug 29 '18 at 0:33
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\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 Aug 29 '18 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 Aug 29 '18 at 12:39
  • \$\begingroup\$ So plain strcpy would be a better alternative here? \$\endgroup\$ – bool3max Aug 29 '18 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 by flagName not fit in (defaults + pos)->name, what functionality do you want? \$\endgroup\$ – chux - Reinstate Monica Aug 29 '18 at 13:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.