Bare-bones string library

Question

After years of criticizing others, I've finally found the time and worked up the courage to polish up one of my bits of code and solicit criticisms of my own.

This is a simple dynamic-string library that I wrote for one of my old C projects (don't judge too harshly if you look into it -- it's a few years old, and I've just begun revising it).

Some notes on the design:

• Its interface is fairly heavily influenced by C++'s std::string
• Internally, it manages a dynamically allocated character buffer
• It is encoding agnostic -- it thinks in bytes, not true characters
• For academic interest, I went ahead and did a naive small string optimization

Please note that this is not intended to be a fully functioning, Swiss army knife of a string library. The functionality is very limited, and that's the way I intended it. All it is used for is corralling an HTTP request response into something a bit safer and more flexible than a bare manually managed c-string. There is a possibility that it may grow more complex though, so anything in the interface that could prohibit that would be of great interest to me.

To my knowledge, it is fully compliant to the C99 standard.

---

string_buffer.h

#ifndef UPMON_STRING_BUFFER_H
#define UPMON_STRING_BUFFER_H

#ifdef  __cplusplus
extern "C" {
#endif

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

typedef struct StringBuffer {
    char* str;
    size_t len;
    size_t cap;
    char small_str[64];
} StringBuffer;

void string_buffer_init(StringBuffer* s);

void string_buffer_cleanup(StringBuffer* s);

const char* string_buffer_cstr(const StringBuffer* s);

size_t string_buffer_length(const StringBuffer* s);

bool string_buffer_set_bytes(StringBuffer* s, const char* str, size_t len);
bool string_buffer_set_cstr(StringBuffer* s, const char* str);
bool string_buffer_set_string_buffer(StringBuffer* dst, const StringBuffer* src);

bool string_buffer_append_bytes(StringBuffer* s, const char* str, size_t len);
bool string_buffer_append_cstr(StringBuffer* s, const char* str);
bool string_buffer_append_string_buffer(StringBuffer* dst, const StringBuffer* src);

void string_buffer_clear(StringBuffer* s);

#ifdef  __cplusplus
}
#endif

#endif

---

string_buffer.c

#include "string_buffer.h"

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

void string_buffer_init(StringBuffer* s) {
    s->small_str[0] = '\0';
    s->str = NULL;
    s->len = 0;
    s->cap = sizeof(s->small_str);
}

void string_buffer_cleanup(StringBuffer* s) {
    free(s->str);
    s->len = 0;
    s->cap = sizeof(s->small_str);
}

const char* string_buffer_cstr(const StringBuffer* s) {
    return (s->cap <= sizeof(s->small_str)) ? s->small_str : s->str;
}

static char* string_buffer_buf(StringBuffer* s) {
    return (char*) string_buffer_cstr(s);
}

size_t string_buffer_length(const StringBuffer* s) {
    return s->len;
}

// Currently this is a hidden function, but if ever necessitated, it can be
// exposed in the interface.
static bool string_buffer_reserve(StringBuffer* s, size_t min_cap) {
    if (s->cap >= min_cap) {
        return true;
    }

    char* new_buf = realloc(s->str, min_cap);
    if (new_buf == NULL) {
        return false;
    }

    // If we're moving from small_str to a buffer, we need to copy over the small_str.
    if (s->str == NULL) {
        memcpy(new_buf, s->small_str, sizeof(s->small_str));
    }

    s->str = new_buf;
    s->cap = min_cap;

    return true;
}

bool string_buffer_set_bytes(StringBuffer* s, const char* str, size_t len) {
    if (!string_buffer_reserve(s, len + 1)) {
        return false;
    }
    char* buf = string_buffer_buf(s);
    memcpy(buf, str, len);
    buf[len] = '\0';
    s->len = len;
    return true;
}

bool string_buffer_set_cstr(StringBuffer* s, const char* str) {
    return string_buffer_set_bytes(s, str, strlen(str));
}

bool string_buffer_set_string_buffer(StringBuffer* dst, const StringBuffer* src) {
    return string_buffer_set_bytes(dst, string_buffer_cstr(src), string_buffer_length(src));
}

bool string_buffer_append_bytes(StringBuffer* s, const char* str, size_t len) {
    if (!string_buffer_reserve(s, s->len + len + 1)) {
        return false;
    }
    char* dst = string_buffer_buf(s) + s->len;
    memcpy(dst, str, len);
    dst[len] = '\0';
    s->len += len;
    return true;
}

bool string_buffer_append_cstr(StringBuffer* s, const char* str) {
    return string_buffer_append_bytes(s, str, strlen(str));
}

bool string_buffer_append_string_buffer(StringBuffer* dst, const StringBuffer* src) {
    return string_buffer_append_bytes(dst, string_buffer_cstr(src), string_buffer_length(src));
}

void string_buffer_clear(StringBuffer* s) {
    s->len = 0;
    string_buffer_buf(s)[0] = '\0';
}

Answer 1

Overall, I think this is very well done. There is one major thing I find wrong with it though:

Where is the documentation?!?!?!

Sure, as a developer using this library I could read through your short source file and pick it apart to figure out how everything should work. But my time is better spent elsewhere, reading the documentation of other larger projects and programming my own code. That could actually even be a breaking point between using this library, and using a similar (even inferior) library that had good documentation for me to read.

Answer 2

1. Calling code is not limited to never calling string_buffer_cleanup() after it has been called. A defensive routine would allow repeated calls.

   void string_buffer_cleanup(StringBuffer* s) {
     free(s->str);
     string_buffer_init(s);
   }
   

2. string_buffer_reserve() does not appear to shift from allocated memory to using the small buffer if able. Code does so going from small buf to allocated memory. This asymmetry, IMO, should be eliminated.

3. Since code is using #ifdef __cplusplus extern "C" { #endif, suggest tagging post as C++ also.

4. Pedantic code would insure s->len + len + 1 (and others) do not overflow.

5. As this code allows string_buffer_set_bytes() which may have embedded '\0', the results of string_buffer_length(s) may not equal strlen(string_buffer_cstr(s)).

Answer 3

I'd declare

struct StringBuffer_s {
    ....
};

straight in StringBuffer.c, so that StringBuffer.h would have just

typedef struct StringBuffer_s StringBuffer;

to isolate client code from implementation changes.

Answer 4

All it is used for is corralling an HTTP request response into something a bit safer and more flexible than a bare manually managed C-string.

Therefore I will be considering safety concerns, if you don't mind.

string_buffer.h

1. __cplusplus: I don't know whether this string library is used in C++ too or not. Either way, I would personally avoid that: C++ has std::string. And if, for some reason, you are using this library to handle strings...just don't again. std::string is likely to outperform your library.

2. StringBuffer declaration: You can omit the struct name when using typedef. I changed len and cap to length and capacity, as they look clearer.

   typedef struct {
       char* str;
       size_t length;
       size_t capacity;
       char small_str[64];
   } StringBuffer;
   

---

string_buffer.cpp

1. I do like the small_str which cuts down on small allocations. When you realloc the pointer, though, you just ask for min_cap. After the memcpy, length == capacity.

   Thus, I would recommend you to allocate more bytes e.g. min_cap + sizeof(small_str) ^*. You can find this useful if the library often handles big strings.

^* I would turn sizeof(small_str) into a constant, kind of like default_size, since it appears frequently.

---

Overall design

1. There is no check for NULL pointers. That's gonna lead to lots of SIGSEGVs if you accidentally forget to call string_buffer_init, for example.

2. It's important to point out that you're actually using a function which belongs to an external library.

   Nevertheless, I'd go for:

   • stringbuffer
   • stringBuffer

   • sb rather than string_builder.

     It should be a compact name; consider e.g. glPushMatrix() or alcOpenDevice.