Append Buffer for Buffering Small write(2) Calls

Question

The purpose of this is written in the comment below (it is meant to be used in a text editor):

abuf.h:

#ifndef ABUF_H
#define ABUF_H 1

#include "stddef.h"

/* We define a simple 'append buffer', that is a heap allocated string
 * where we can append to. This avoids calling write() every time we have
 * to print something (e.g. escape sequences) while refreshing the screen,
 * and any flickering effects. */
typedef struct abuf {
    char *buf;          /* A pointer to a chunk of allocated memory. */
    size_t count;       /* Total bytes occupied. */
    size_t capacity;    /* Total bytes allocated. */
} abuf;

[[gnu::hot]] bool append_abuf(abuf ab[restrict static 1],
                              size_t size,                   
                              const char s[restrict static size]);

void abuf_free(abuf ab[static 1]);

#endif /* ABUF_H */

abuf.c:

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

#define GROW_CAPACITY(capacity) \
    (size_t) ((capacity) < 8 ? 8 : (capacity) * 2)

bool abuf_append(abuf ab[restrict static 1],
                 size_t size,
                 const char s[restrict static size])
                           
{
    if ((ab->count + size) > ab->capacity) {
        ab->capacity = GROW_CAPACITY(ab->capacity);
        void *const tmp = realloc(ab->buf, ab->capacity);

        if (tmp == nullptr) {
            return false;
        }
        ab->buf = tmp;
    }
    memcpy(ab->buf + ab->count, s, size + 1);
    ab->count += size;
    return true;
}

void abuf_free(abuf ab[static 1]) 
{
    free(ab->buf);
}

And it is used like this:

abuf ab = {};
ssize_t exit_stat;

const bool error = !abuf_append(...) 
                   || !abuf_append(...) 
                   || !draw_rows(...)
                   || !draw_status_bar(...) 
                   || !draw_msg_bar(...) 
                   || !abuf_append(...) 
                   || !abuf_append(...);

if (!error
    && !(exit_stat = write_eintr(STDOUT_FILENO, ab.buf, ab.count))) {
    perror("write()");
}
abuf_free(&ab);
return exit_stat;

Review Request:

Anything, everything.

Should I add the two missing overflow checks in abuf_append() when adding?

Answer 1

if ((ab->count + size) > ab->capacity) 
 //... increase capacity
memcpy(ab->buf + ab->count, s, size + 1);

if count+size == capacity you write past the end of the allocation.

In abuf_append, is s supposed to be a null terminated string? Copying size+1 implies that, but it's not stated.
Consider explicitly null terminating any passed-in string.

Is size supposed to be the string length? Maybe name it that?

Consider using memmove instead of memcpy to prevent UB if callers make the unfortunate choice of passing ab->s into append_abuf

And is it append_abuf or abuf_append? your header and implementation don't agree.

Consider an abuf_init with an initial capacity for uses like your example where you know beforehand the minimum number of characters.

Answer 2

I think that abuf_append() really needs a [[nodiscard]] attribute. If the function doesn't manage to perform the requested action, the addend could easily be lost if the caller ignores the return value.

---

A factor 2 growth rate is probably a bit large - common realloc strategies tend towards capacity * 3 / 2 or thereabouts. I'd prefer to see next_capacity() as a (static) function rather than a macro.

---

We have a bug when realloc() fails - after we have increased capacity. Don't store the new capacity until we have successfully reallocated.

---

We can save on allocations if users are able to reset an existing buffer - perhaps call that abuf_clear()?.

We could be more robust against use-after-free by setting buf = nullptr and count = capacity = 0u in abuf_free(). Then the buffer is still in a valid state and can be used again.

Answer 3

short circuiting

The repeated || idiom is fascinating. I guess my brain just doesn't work that way. Wouldn't it be more convenient to state it in the positive?

const bool success = abuf_append(...) 
                     && abuf_append(...) 
                     && draw_rows(...)
                     && draw_status_bar(...) 
                     && draw_msg_bar(...) 
                     && abuf_append(...) 
                     && abuf_append(...);

if (success && ...

And then making caller worry about perror("write()") seems inconvenient. Better to package that up in a tiny helper which will also do the free().

micro-optimization

[[gnu::hot]] bool append_abuf( ...

This impresses me as just being noise.

If benchmarking revealed that it made a noticeable difference, then please write a comment that mentions the results, or URL of a benchmark run, so future maintenance engineers won't just rip it out. As written the OP code is making a claim "hot improves things" which currently I'm just not believing.

The GCC documentation explains:

When profile feedback is available, via -fprofile-use, hot functions are automatically detected and this attribute is ignored.

I feel this is a more sane way of expressing our desires and getting the compiler to implement them, in a maintainable way. That is, better to push the details down into the build process. That way the right thing will happen when usage patterns change next year.