Splitting C string without modifying input

Question

I wrote this after using strtok_r and needing something similar that can copy the input string and handle multiple characters in the delimiter. Its functionally similar to Python's str.split(), as it keeps empty spaces without trimming.

tokens.h

#ifndef TOKENS_H
#define TOKENS_H

#include <string.h>

struct Tokens {
    char **array;
    size_t count;
};

extern struct Tokens *string_split(const char *input,
                                   const char *delim,
                                   const int count);

extern void tokens_free(struct Tokens *tokens);

#endif

tokens.c

#include "tokens.h"

#include <stdbool.h>
#include <stdlib.h>

static bool tokens_add(struct Tokens *tokens, const char *token){
    tokens->array = realloc(tokens->array,
                            ++tokens->count * sizeof(char *));

    if (!tokens->array){
        tokens->count--;

        return false;
    }

    char *dupstr = strdup(token);

    if (!dupstr){
        tokens->count--;

        return false;
    }

    tokens->array[tokens->count - 1] = dupstr;

    return true;
}

struct Tokens *string_split(const char *input,
                            const char *delim,
                            const int count){

    struct Tokens *tokens = malloc(sizeof(struct Tokens));

    if (!tokens){
        return NULL;
    }

    tokens->count = 0;
    tokens->array = NULL;

    const size_t inputlen = strlen(input);
    const size_t delimlen = strlen(delim);

    if (count == 0 || delimlen == 0){
        tokens_add(tokens, input);

        return tokens;
    }

    int delimcount = 0;
    size_t inputpos = 0;
    const char *start = NULL;

    while ((start = strstr(&input[inputpos], delim))){
        const int delimpos = (start - &input[inputpos]);
        char token[delimpos + 1];

        memcpy(token, &input[inputpos], delimpos);
        token[delimpos] = '\0';

        if (!tokens_add(tokens, token)){
            return NULL;
        }

        inputpos += (delimpos + delimlen);

        if (delimcount++ == count){
            break;
        }
    }

    if (inputpos <= inputlen){
        const int charcount = (inputlen - inputpos);
        char token[charcount + 1];

        memcpy(token, &input[inputpos], charcount);
        token[charcount] = '\0';

        if (!tokens_add(tokens, token)){
            return NULL;
        }
    }

    return tokens;
}

void tokens_free(struct Tokens *tokens){
    if (!tokens){
        return;
    }

    for (size_t index = 0; index < tokens->count; index++){
        free(tokens->array[index]);
    }

    free(tokens);
}

test.c

#include <stdio.h>

#include "tokens.h"

int main(void){
    // <0 = split indefinitely
    struct Tokens *tokens = string_split("Hello;world;!;", ";", -1);

    if (tokens){
        for (size_t index = 0; index < tokens->count; index++){
            printf("%s\n", tokens->array[index]);
        }

        tokens_free(tokens);
    }
}

Answer 1

token_add()

• do not change the struct members before you know that your call will succeed, otherwise you'll end up with inconsistent state (e.g. you're overwriting tokens->array if realloc fails). That means keep the increment of count and the assignment of array until the end of the function.
• Note that strdup is not a C standard function.

split_string()

• you're creating a temporary copy in the local variables token. If you'd pass the pointer and the length to token_add and you could safe the program a string copy.

• there is no need to have delimpos in string_split being const. Same goes for the parameter count and charcount. As a rule of thumb, dont use const for integer types, booleans and floating points.

• Note: your comment in test.c states that values less than 0 mean unlimited matching, but your code doesn't contribute to that statement. If you have enough matches you'll eventually increment delimcount to match -1.
• Separate the increment of delimcount from the comparison with count. Some will ask himself if this is correct and right now, I couldn't tell ;-) Why don't you use tokens->count?
• If a call to tokens_add failes, you're returning NULL from split_string without freeing the Tokens struct allocated before, leaking its memory.

misc

• Provide more test cases. One test case for handling unlimited matching, one for restricted matching, one for empty delimiters. Best is to write down what your API supports and then add a test for every special case.

For instance, if I test the following:

    struct Tokens *tokens = string_split("Hello;world;!;", ";", 2);

    assert(tokens->count == 2);
    assert(strcmp("Hello", tokens->array[0]) == 0);
    assert(strcmp("world!", tokens->array[1]) == 0);

I receive: test: test.c:22: test_2: Assertion 'tokens->count == 2' failed.

• you don't need the include in tokens.h, it's sufficient if you add it to tokens.c

Answer 2

tokens.h

Instead of <string.h> use <stddef.h>, you only need access to size_t.

tokens.c

Now you need, <string.h> :-)

Modify interface to static bool tokens_add(struct Tokens *const tokens, const char *const token, size_t toklen). This we remove the need for extra copying in split_string.

Instead of the non-standard strdup you now use

char *dupstr = malloc(toklen + 1u);
if (dupstr == NULL) {
    return false;
}
memcpy(dupstr, token, toklen);
dupstr[toklen] = '\0';

In string_split possibly start with this check

if (input == NULL || delim == NULL) {
    return NULL;
}

later the check for zero count or delimlen led to a memory leak in the case of tokens_add failure.

if (count == 0 || delimlen == 0) {
    if (tokens_add(tokens, input, inputlen)) {
        return tokens;
    } else {
        tokens_free(tokens);
        return NULL;
    }
}

The start of the while-loop can now be simplified to

    const int delimpos = start - &input[inputpos];
    if (!tokens_add(tokens, &input[inputpos], delimpos)) {
        tokens_free(tokens);
        return NULL;
    }

since no token temporary buffer copy is needed (using VLA, a risk for stack overflow).

And the end

if (inputpos <= inputlen) {
    if (!tokens_add(tokens, &input[inputpos], inputlen - inputpos)) {
        tokens_free(tokens);
        return NULL;
    }
}

Finally, in tokens_free you have a memory leak. You are missing

free(tokens->array);