3
\$\begingroup\$

I've some functions targeted to simplify working with strings in some of my other projects. I'd like feedback on this code and whether or not the implementation is efficient and memory safe. I work with both normal strings (char*) and string arrays (char**) in this code.

Here's stringfuncs.c

#include<stdio.h>
#include<stdlib.h>
#include<string.h>
#include "stringfuncs.h"

#define ALLOC_FAIL(ptr, location, alloc_type) \
        if (ptr == NULL) \
        { \
            fprintf(stderr, #alloc_type " failed in " #location); \
            abort(); \
        }

char* str_append(char element, char* str, const int end_index, int* size)
{
    // Inserts a char to given string at given index
    if (end_index == *size)
    {
        // Reallocate if needed
        str = realloc(str, (*size *= 2) * sizeof(*str));
        if (str == NULL)
        {
            fprintf(stderr, "realloc failed in str_append");
            abort();
        }
    }
    str[end_index] = element;
    return str;
}

char** strarr_append(char* elementstr, char** strarr, const int end_index, int* size)
{
    // Inserts a string to given array at given index
    if (end_index == *size)
    {
        // Reallocate if needed
        strarr = realloc(strarr, (*size *= 2) * sizeof(*strarr));
        if (strarr == NULL)
        {
            fprintf(stderr, "realloc failed in str_append");
            abort();
        }
    }
    strarr[end_index] = elementstr;
    return strarr;
}

char* trunc_string(char* str, const int end_index)
{
    // Reallocate string for the amount of memory it needs
    str = realloc(str, (end_index + 1) * sizeof(*str));
    // Null terminate the string
    str[end_index] = '\0';
    return str;
}

char** trunc_strarray(char** strarr, const int index)
{
    // Reallocate string array for the amount of memory it needs
    strarr = realloc(strarr, (index + 1) * sizeof(*strarr));
    return strarr;
}

char* get_string(const char* prompt)
{
    // A function to get string user input
    int index, size = 1;
    char element;
    char* string = malloc(size * sizeof(*string));
    ALLOC_FAIL(string, get_string, malloc);
    // Print the given prompt
    printf("%s", prompt);
    for (index = 0; (element = getchar()) != EOF && element != '\n'; index++)
    {
        // Record every character input until user presses enter (and or we encounter EOF)
        string = str_append(element, string, index, &size);
    }
    // Truncate and null terminate the string
    string = trunc_string(string, index);
    return string;
}

char** split_string(const char delimiter, const char* string, int* length)
{
    // Variables to keep track of splitarr
    int arrsize = 2, arrindex = 0;
    // Variables to keep track of elementstr
    int strsize = 2, strindex = 0;
    // Set up splitarr and elementstr with an initial size;
    char** splitarr = malloc(arrsize * sizeof(*splitarr));
    ALLOC_FAIL(splitarr, split_string, malloc);
    char* elementstr = malloc(strsize * sizeof(*elementstr));
    ALLOC_FAIL(elementstr, split_string, malloc);
    for (int index = 0; string[index] != '\0'; strindex++, index++)
    {
        if (string[index] == delimiter)
        {
            // elementstr ends here
            // Truncate and null terminate the string
            elementstr = trunc_string(elementstr, strindex);
            // Add string to string array
            splitarr = strarr_append(elementstr, splitarr, arrindex, &arrsize);
            arrindex++;
            // Cleanup
            strsize = 1;
            strindex = -1;
            elementstr = realloc(NULL, strsize * sizeof(*elementstr));
            ALLOC_FAIL(elementstr, split_string, realloc);
        }
        else
        {
            // non-delimiter character, append to elementstr
            elementstr = str_append(string[index], elementstr, strindex, &strsize);
        }
    }
    // Truncate and null terminate the final string
    elementstr = trunc_string(elementstr, strindex);
    // Add final string to string array
    splitarr = strarr_append(elementstr, splitarr, arrindex, &arrsize);
    // Truncate the string array
    splitarr = trunc_strarray(splitarr, arrindex);
    // Assign the length of the array
    *length = arrindex + 1;
    return splitarr;
}

char** destroy_strarr(char** strarr, int length)
{
    // Free all strings inside an array of strings and the array itself
    int index = 0;
    while (index < length)
    {
        // Free the elements and assign the pointer to NULL
        free(strarr[index]);
        strarr[index++] = NULL;
    }
    // Free the array itself and assign to NULL
    free(strarr);
    strarr = NULL;
    return strarr;
}

Here's the corresponding stringfuncs.h

#pragma once

/*
Take string input from user
Pass in a string prompt to display to the user prior to input
Returns a pointer to the input string
*/
char* get_string(const char* prompt);

/*
Split given string by delimiter into an array of strings
Pass in the address of a variable to store the length of the array
Returns a pointer to the array of strings
*/
char** split_string(const char delimiter, const char* string, int* length);

/*
Free all the memory used by an array of strings
Assigns all the string elements as NULL
Returns NULL on success
*/
char** destroy_strarr(char** strarr, int length);

And example use-

#include<stdio.h>
#include<stdlib.h>
#include "stringfuncs.h"

int main()
{
    int length;
    char* input = get_string("> ");
    char** strarr = split_string(' ', input, &length);
    strarr = destroy_strarr(strarr, length);
    free(input);
    input = NULL;
    return 0;
}

Primarily concerned about split_string and get_string, the rest are helpers.

Note: This targets C only, not C++

\$\endgroup\$
3
\$\begingroup\$

whether or not the implementation is efficient and memory safe.

memory safe weakness: int vs. size_t

With long strings, length is limited to INT_MAX yet should limit to SIZE_MAX. Recommend size_t size, end_index.

memory safe weakness: size extreme range

Better code would handle size == 0 and detect when size * 2 overflows.

// str = realloc(str, (*size *= 2) * sizeof(*str));
if (size == 0) size = 2;
else if (size <= SIZE_MAX/2) size *= 2;
else tbd_code();
str = realloc(str, size * sizeof(*str));

Memory efficient strength: allocation

Good to use exponential allocation growth of size

Maintenance efficient strength: sizeof *ptr

sizeof(*strarr) or sizeof *strarr easier to code right, review and maintain than sizeof (some_type)

Functional weakness: get_string() and EOF

When end-of-file (and nothing read), code returns an empty string. This is indistinguishable from first reading a null character.

When a rare input error, there is no indication of a problem. Code simply forms a string of characters read up to that point.

Perhaps return NULL on those cases instead.

Memory safe strength: destroying NULL

free() allows free(NULL). destroy_strarr(NULL,0) is allowed: good.

Memory safe weakness: missing free strategy

stringfuncs.h should outline what needs to be free'd and how. Assume user of your good code only sees the .h file.

General feedback

  • Namespace of functions should be made uniform. Recommend prefix that matches .h file name.

  • #pragma once ubiquitous, but not standard C.

  • fprintf(stderr, #alloc_type " failed in " #location) deserves a '\n'.

  • I am tempted to put char *str, size_t end_index, size_t size in a struct.

  • const in const char delimiter of split_string() declaration serves no purpose.

  • Private functions in stringfuncs.c should be static.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks! Could you please elaborate where in the header should I include the free strategy? Should I just include it in the function docstring? \$\endgroup\$ – Chase Apr 20 at 9:53
  • \$\begingroup\$ @Chase Either/Both. \$\endgroup\$ – chux - Reinstate Monica Apr 20 at 14:24
2
\$\begingroup\$

A note on standards

My suggestions below - around errno and getline - work off of the POSIX standard, which adds more functionality than the bare C standard. If you are in a Mac or Unix-like environment this will be accessible to you. Other environments like Windows can pull parts of it in depending on which compiler you use.

errno

This:

    if (ptr == NULL) \
    { \
        fprintf(stderr, #alloc_type " failed in " #location); \

only offers you partial coverage. The spec says that

Otherwise [when unsuccessful], it shall return a null pointer and set errno to indicate the error.

This means you are better-off calling perror. The same applies elsewhere, for example when you check realloc.

Output simplification

Sometimes the compiler will do this for you, but I still find it's a good idea to replace

printf("%s", prompt);

with

puts(prompt);

Getting a line

I think most of get_string is unnecessary. Have a read through getline.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ printf("%s", prompt); has a different functionality than puts(prompt);. Consider fputs(prompt, stdout); instead. \$\endgroup\$ – chux - Reinstate Monica Apr 19 at 22:01
  • 1
    \$\begingroup\$ @chux-ReinstateMonica Thanks for keeping me honest. Edited. \$\endgroup\$ – Reinderien Apr 22 at 2:29

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.