3
\$\begingroup\$

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?

\$\endgroup\$

3 Answers 3

3
\$\begingroup\$
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.

\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ I can't remember how many times I've written reallocation code exactly like this. And it always goes unnoticed because I fail on an out-of-memory condition. It is even more surprising that it has never been caught here until now (on my posts, of course). Thank you. \$\endgroup\$
    – Harith
    Commented May 22 at 14:11
  • 2
    \$\begingroup\$ Actually, a growth factor of 2 is very common. Rust Vec uses 2, for example, as do stdlibc++ and libc++. There are arguments that the golden ratio would be better (of which 1.5 is a decent approximation), but few standard libraries bothered switching. \$\endgroup\$ Commented May 23 at 12:05
3
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

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

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