3
\$\begingroup\$

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);
    }
}
\$\endgroup\$
2
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ After going through every point, not only has this been humbling but it helped me fix up stuff I missed in my other projects as well! My only question is why avoid const on those types? Other than that, the assertion failing there is intentional since it's made to follow Python's own implementation which applies the count to how many delimiters it goes through (which ends up with 2 tokens plus the rest of the string). Thank you for the review because it's humbling and helpful! \$\endgroup\$ – ifhaoynf Feb 24 at 22:01
  • 1
    \$\begingroup\$ For const: clearly readability. In an API or simply in a function prototype you can use const to communicate, that you're not going to change the input value. In function bodies it is rather uncommon, so you're only adding a little moment where the reader struggles and thinks "moment, why is this const?". \$\endgroup\$ – Cornholio Feb 25 at 6:01
1
\$\begingroup\$

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);
\$\endgroup\$
  • \$\begingroup\$ Thanks! I was actually torn between stddef.h and string.h in the header file but I wasn't sure if I should avoid adding another header since I use string.h's functions or not. As for everything else, I combined it with the review from Cornholio and you guys have helped a lot! \$\endgroup\$ – ifhaoynf Feb 24 at 22:04
  • 1
    \$\begingroup\$ Good, didn't want to add too much of the same. \$\endgroup\$ – Bo R Feb 24 at 23:07

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.