10
\$\begingroup\$

Lots of languages have something that lets you build up a dynamically-sized string with minimal overhead. C doesn't, and I found myself using code that did that manually in a couple of places, so I packaged it into a class. Note that I've only implemented functionality I'm using.

stringbuilder.h:

#ifndef CONCATEN_STRINGBUILDER_H
#define CONCATEN_STRINGBUILDER_H

#include <stddef.h>
#include <stdbool.h>

struct stringbuilder_s;
typedef struct stringbuilder_s *stringbuilder_t;
stringbuilder_t sb_new(size_t);
bool sb_append(stringbuilder_t, char);
char *sb_as_string(stringbuilder_t);
void sb_free(stringbuilder_t);

#endif //CONCATEN_STRINGBUILDER_H

stringbuilder.c:

#include <stdlib.h>
#include <string.h>
#include "stringbuilder.h"

struct stringbuilder_s {
    char *mem;
    size_t count;
    size_t cap;
};
typedef struct stringbuilder_s *stringbuilder_t;
stringbuilder_t sb_new(size_t init_cap) {
    stringbuilder_t ret = malloc(sizeof(struct stringbuilder_s));
    if (!ret) return NULL;
    ret->mem = calloc(init_cap, sizeof(char));
    if (!ret->mem) return NULL;
    ret->cap = init_cap;
    ret->count = 0;
    return ret;
}
#define LOAD_FACTOR 2
bool sb_append(stringbuilder_t to, char c) {
    to->mem[to->count] = c;
    ++to->count;
    if (to->count == to->cap) {
        char *new_mem = realloc(to->mem, to->cap * LOAD_FACTOR);
        if (!new_mem) {
            return false;
        }
        memset(new_mem + to->cap, 0, to->cap);
        to->mem = new_mem;
        to->cap *= LOAD_FACTOR;
    }
    return true;
}
char *sb_as_string(stringbuilder_t sb) {
    return sb->mem;
}
void sb_free(stringbuilder_t sb) {
    free(sb->mem);
    free(sb);
}

I'm interested specifically in:

  • Performance. This code gets called a lot. I want it to be as fast as possible.
  • Memory safety. While I'm fairly sure that this doesn't leak memory (assuming it's used properly), I'm not confident, and I'm not sure how to check.
  • Edge cases. It works, as far as I can tell, but that doesn't mean that it's bug-free.

Tested here.

\$\endgroup\$
4
  • \$\begingroup\$ If anyone's got better ideas for tags, feel free to edit them. I haven't been here in a while, and I don't know them anymore. \$\endgroup\$
    – Nic
    Feb 14, 2017 at 1:47
  • 1
    \$\begingroup\$ Those tags look good for the question. \$\endgroup\$
    – Phrancis
    Feb 14, 2017 at 2:19
  • 1
    \$\begingroup\$ I think there's a potential memory leak in sb_new: if ret = malloc(...) succeeds, but ret->mem = calloc(...) fails (returns NULL), the memory at ret is never freed. \$\endgroup\$ Apr 14, 2017 at 0:56
  • 1
    \$\begingroup\$ @JonaChristopherSahnwaldt Oh, yep, good catch. I've changed my code significantly since writing this, though. \$\endgroup\$
    – Nic
    Apr 14, 2017 at 1:16

3 Answers 3

7
\$\begingroup\$

Bug: Initial allocation not cleared to zero

Your initial allocation of sb->mem uses malloc instead of calloc, so its contents are uninitialized. If you then append a few characters and call sb_as_string(), you will get back a string that is not properly terminated. You should use calloc instead.

Minor bug

If your call to realloc fails, your buffer will be incorrect because it will no longer be null terminated (you just appended a character to the last spot). You should either rewrite a '\0' to the end of the buffer if realloc fails, or do the realloc before you append the character.

Argument check

When creating a string buffer, you should handle the case where init_cap is passed in as 0. You can set it to some default value in that case. Right now, an initial capacity of 0 will cause a crash down the line because your append function will append to a zero length buffer without ever reallocating.

Usability

I would much prefer an append function that took a string argument instead of a character argument. I'm not sure that I would ever need to append one character at a time.

Also, it might be nice to have a to_string type function that returns the string but also frees the stringbuilder. The way you currently have it, you can retrieve the string, but if you subsequently free the stringbuilder it will also free the string you just retrieved. That makes it difficult to use the string because its lifetime is tied to the lifetime of the stringbuilder.

\$\endgroup\$
3
  • \$\begingroup\$ For that first bug -- dangit, I spotted it in sb_append, but forgot about the similar code in sb_new. For the last thing -- like I mentioned, I'm writing the functions I'm using. As it happens, the places where I'm using this only ever give characters one at a time, and I'm using this to store the characters that some other code decides should be stored (i.e. I'm extracting data from a character-by-character source). For the rest, I'll fix it now. Thanks! \$\endgroup\$
    – Nic
    Feb 14, 2017 at 2:32
  • \$\begingroup\$ @QPaysTaxes I added another usability remark about being able to retrieve the string and free the stringbuilder at the same time. \$\endgroup\$
    – JS1
    Feb 14, 2017 at 2:49
  • \$\begingroup\$ Hm, that raises a good point, actually. Right now I'm doing a strncopy where I'm getting the string value, but it'd make more sense to either copy it in sb_as_string or offer a version that returns a copied string. I'll add that in, too. Thanks! \$\endgroup\$
    – Nic
    Feb 14, 2017 at 2:52
3
\$\begingroup\$

To be more comprehensive, the line in sb_append():

memset(new_mem + to->cap, 0, to->cap);

should become:

memset(new_mem + to->cap, 0, (to->cap) * (LOAD_FACTOR - 1));

That matters when LOAD_FACTOR is set to something greater than 2.

thanks for sharing this code.

\$\endgroup\$
2
  • \$\begingroup\$ Or, equivalently, compute new_cap = to->cap * LOAD_FACTOR, and then memset(new_mem + to->cap, 0, new_cap - to->cap). Good catch. \$\endgroup\$ Sep 18, 2018 at 14:14
  • \$\begingroup\$ Oh, and welcome to Code Review! \$\endgroup\$ Sep 18, 2018 at 14:54
1
\$\begingroup\$

I don't think it's necessary to insist that the string builder itself be in dynamic storage. It's a fixed size, and would really like to be in automatic storage. Ideally, it would be re-usable, too.

That can be enabled, by adding functions like these:

typedef struct stringbuilder_s stringbuilder_s;

bool sb_init(stringbuilder_s *b, const char *s)
{
    b->count = strlen(s);
    b->cap = LOAD_FACTOR * (b->count + 1);
    b->mem = malloc(b->cap);
    if (!b->mem) {
        b->count = 0;
        b->cap = 0;
        return false;
    }
    strcpy(b->mem, s);
    return true;
}

void sb_close(stringbuilder_s *b)
{
    free(b->mem);
    b->cap = 0;
    b->count = 0;
}

We can then use like this:

#include <stdio.h>

int main(void)
{
    stringbuilder_s builder;
    if (!sb_init(&builder, "foo")) { goto fail; }
    if (!sb_append_char(&builder, '1')) { goto fail; }
    printf("Created %s\n", sb_as_string(&builder));
    sb_close(&builder);
    return 0;

 fail:
    sb_close(&builder);
    fprintf(stderr, "String creation failed");
    return 1;
}

Bug:

  • If sb_new() fails to allocate mem, we really need to free ret before returning null.

Minor points:

  • sizeof (char) is 1 by definition, and sizeof *ret is clearer than sizeof (struct stringbuilder_s) when allocating to assign to ret.
  • It's wasteful to zero out the whole capacity, when we only need a single terminator for a string. We could defer the null termination until we sb_as_string() is called, but I think it's better to keep mem nul-terminated as we go. (The overhead shrinks once we support sb_append_string(), of course).
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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