16
\$\begingroup\$

I'm writing a function to stringify a nested struct. I'm using snprintf to pre-calculate how much space I'll need to allocate.

The problem is, it's extremely repetitious, and will only get worse if I ever need to add more fields to any of the structs. The duplication also creates more of an opportunity for accidental asymmetrical changes.

Is there an idiomatic way of reducing the duplication?

The repetitious code:

char* stringify_state(State* state) {
    Pet* p = state->pet;
    Settings* s = state->settings;

    char* format_string = "%lld %f %f %f %f %f %f %f %f %f %f";

    size_t len = snprintf(NULL, 0, format_string,
                          state->last_update_time,
                          p->health, p->max_health, p->satiation, p->max_satiation,
                          s->milliseconds_per_tick, s->hunger_pain_per_tick, s->hunger_per_tick, s->satiated_heal_per_tick,
                          s->pain_per_wrong_answer, s->satiation_per_right_answer);

    char* buff = malloc(len + 1);

    if (buff) {
        snprintf(buff, len + 1, format_string,
                 state->last_update_time,
                 p->health, p->max_health, p->satiation, p->max_satiation,
                 s->milliseconds_per_tick, s->hunger_pain_per_tick, s->hunger_per_tick, s->satiated_heal_per_tick,
                 s->pain_per_wrong_answer, s->satiation_per_right_answer);

        return buff;

    } else {
        return NULL;
    }
}

Context (Not what I'm asking about, but feel free to mention anything):

#include <time.h>
#include <stdlib.h>
#include <stdio.h>

typedef struct {
    double health;
    double satiation;

    double max_health;
    double max_satiation;

} Pet;

void init_pet(Pet* pet, double max_health, double max_satiation) {
    pet->max_health = max_health;
    pet->max_satiation = max_satiation;

    pet->health = max_health;
    pet->satiation = max_satiation;



typedef struct {
    double milliseconds_per_tick;

    double hunger_pain_per_tick;
    double hunger_per_tick;

    double satiated_heal_per_tick;

    double pain_per_wrong_answer;
    double satiation_per_right_answer;

} Settings;

void init_settings(Settings* s,
                   double ms_per_tick,
                   double hung_pain_per_tick, double hung_per_tick,
                   double sat_heal_per_tick,
                   double pain_per_wrong, double sat_per_right) {

    s->milliseconds_per_tick = ms_per_tick;

    s->hunger_pain_per_tick = hung_pain_per_tick;
    s->hunger_per_tick = hung_per_tick;

    s->satiated_heal_per_tick = sat_heal_per_tick;

    s->pain_per_wrong_answer = pain_per_wrong;
    s->satiation_per_right_answer = sat_per_right;
}



typedef struct {
    time_t last_update_time;
    Pet* pet;
    Settings* settings;

} State;

void init_state(State* state, Pet* pet, Settings* settings, time_t t) {
    state->pet = pet;
    state->settings = settings;

    state->last_update_time = t;
}

A test main:

int main() {
    Pet pet;
    init_pet(&pet, 100, 100);

    Settings settings;
    init_settings(&settings, 91, 82, 73, 64, 55, 46);

    State state;
    init_state(&state, &pet, &settings, 0);

    char* str = stringify_state(&state);

    if (str) {
        printf("%s", str);

        free(str);
    }
}

Prints

0 100.000000 100.000000 100.000000 100.000000 91.000000 82.000000 73.000000 64.000000 55.000000 46.000000
\$\endgroup\$
23
\$\begingroup\$

@vnp's code is solid and helpful, but his stringify_state_helper is a single-purpose function, and still leaves a degree of repetition and memory management in stringify_state. I'd rather have general-purpose to_string that takes printf-style arguments, allocates sufficient space for the converted result, and prints into that space, and returns the result:

#include <stdio.h>
#include <stdarg.h>
#include <stdlib.h>

char *to_string(char const *fmt, ...) { 
    va_list args;
    va_start(args, fmt);

    va_list args_dupe;
    va_copy(args_dupe, args);

    int size = vsnprintf(NULL, 0, fmt, args);

    char *ret = malloc(size+1);

    if (ret != NULL)
        vsnprintf(ret, size+1, fmt, args_dupe);

    va_end(args);
    va_end(args_dupe);

    return ret;
}

In fairness, this does require a little more code, and the v*printf functions (and argument-list macros) are a bit less known, so some may find it a bit more difficult to understand.

On the other hand, in exchange for that bit of extra investment, we get something that's more general, and works much more as I think most people would expect--for example, something like this:

char *s = to_string("%d, %d", 1, 2);

Using this, stringify_state should turn out something like this:

char* stringify_state(State* state) {
    Pet* p = state->pet;
    Settings* s = state->settings;

    return to_string("%lld %f %f %f %f %f %f %f %f %f %f",
                     state->last_update_time,
                     p->health, 
                     p->max_health, 
                     p->satiation, 
                     p->max_satiation,
                     s->milliseconds_per_tick, 
                     s->hunger_pain_per_tick, 
                     s->hunger_per_tick,
                     s->satiated_heal_per_tick,
                     s->pain_per_wrong_answer,
                     s->satiation_per_right_answer);
}

Levels of abstraction

It may just be a pet peeve of mine, but I think this divides into cleaner layers of abstraction. With your original code, we have a single function that mixes a fairly high level of abstraction (format a State into a string) with lower levels of abstraction (e.g., memory management).

@vnp's code sort of inverts the layers of abstraction, so we have the higher level of abstraction at the bottom, and the lower level of abstraction above it.

This one gets the layers of abstraction closer to how I'd like to see them: the memory management and general purpose stringifying is at the bottom of the stack, and the more specific higher level concern of Stringifying a State is at an upper layer. So, something like a call graph accurately reflects the levels of abstraction being dealt with in the code.

Other Points

  1. Given the number (and length) of arguments you're passing, I'd prefer to see the arguments passed one per line as I've formatted them above, rather than a variable number per line to justify out to some particular right margin.
  2. Since stringify_state isn't intended to modify the State whose address is being passed, it's probably better to define the parameter as a State const *. Likewise with the local variables, so the first lines would probably be better written something like this:

    char* stringify_state(State const* state) {
        Pet const* p = state->pet;
        Settings const* s = state->settings;        
    
\$\endgroup\$
  • 1
    \$\begingroup\$ Why use va_copy() instead of a second va_start() and re-using args? \$\endgroup\$ – Deduplicator Jun 28 at 15:52
  • 1
    \$\begingroup\$ @Deduplicator: Good question. The honest answer is that I don't know of much reason to prefer one over the other in this case. \$\endgroup\$ – Jerry Coffin Jun 28 at 16:27
  • \$\begingroup\$ Also, va_copy() and va_start() can do a non-trivial amount of work and even allocate resources. Which is the reason va_end() is needed at all. Yet another reason to refactor your to_string(). \$\endgroup\$ – Deduplicator Jun 28 at 16:35
  • 1
    \$\begingroup\$ @Deduplicator: In theory they can, but on a reasonably modern machine, they're typically pretty lightweight. For example: godbolt.org/z/7qxhlT. gcc compiles va_start to slightly less code than va_copy--but clang compiles va_copy to slightly less code (and msvc/x86 compiles them to two instructions apiece, though its rendition of va_copy may be marginally faster, since it's two movs, vs an lea and a mov for va_start). Bottom line: unless you're doing compiler-specific optimization,it's not immediately obvious to me that you're likely to gain much from refactoring this. \$\endgroup\$ – Jerry Coffin Jun 28 at 18:40
  • 4
    \$\begingroup\$ Call the function asprintf, and wrap it in an #ifdef so it only gets compiled on Windows. \$\endgroup\$ – Mark Jun 28 at 21:31
11
\$\begingroup\$

Consider factoring it out into a function:

static size_t stringify_state_helper(State * state, char * buf) {
    return snprintf(buf, ....);
}

char * stringify_state(State * state) {
    size_t len = stringify_state_helper(state, NULL);
    char * buf = malloc(len + 1);
    if (buf) {
        stringify_state_helper(state, buf);
    }
    return buf;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Duh, I don't know why I had such a hard time thinking of that. This is usually the suggestion I give other people -_-. Thanks. \$\endgroup\$ – Carcigenicate Jun 27 at 21:27
  • \$\begingroup\$ @Carcigenicate We are in the same boat, I guess. \$\endgroup\$ – vnp Jun 27 at 21:41
  • \$\begingroup\$ return snprintf(buf, ....); missing detection of the rare case when snprintf(buf, ....); returns a negative value to indicates an error. \$\endgroup\$ – chux - Reinstate Monica Jul 3 at 3:32
9
\$\begingroup\$

The GNU libc already provides asprintf, which avoids exactly the repetition you are concerned about. I haven't looked at the solutions of other operating systems (Windows, BSD, AIX, Solaris, embedded), though I hope they provide something equivalent.

\$\endgroup\$
  • 1
    \$\begingroup\$ It might be worth noting that this is GPL licensed (not LGPL), so may not be appropriate for all projects. \$\endgroup\$ – Toby Speight Jun 28 at 8:24
  • 2
    \$\begingroup\$ @TobySpeight do you have a citation for asprintf being covered by GPL? github.molgen.mpg.de/git-mirror/glibc/blob/master/stdio-common/… seems to indicate it's licensed under Lesser GPL as is libc in general \$\endgroup\$ – Foon Jun 28 at 18:24
  • 1
    \$\begingroup\$ Writing one's own implementation of asprintf with snprintf is fairly straightforward anyway, and that's basically what Jerry Coffin's answer does. \$\endgroup\$ – jamesdlin Jun 28 at 18:50
  • 3
    \$\begingroup\$ asprintf is so phenomenally useful that there's not a chance it's part of Microsoft's standard C library. \$\endgroup\$ – Mark Jun 28 at 21:33
  • 2
    \$\begingroup\$ @Foon, it seems my recollection has become out of date; it was relicensed to LGPL in 2001. Sorry for the misinformation \$\endgroup\$ – Toby Speight Jul 2 at 12:48
7
\$\begingroup\$

Write the ifs such that the error paths are inside them, and the error-free path isn't indented:

buff = malloc(len + 1);
if (buff) {
        snprintf(buff, len + 1, format_string,
                        state->last_update_time,
                        p->health, p->max_health,
                        p->satiation, p->max_satiation,
                        s->milliseconds_per_tick,
                        s->hunger_pain_per_tick,
                        s->hunger_per_tick,
                        s->satiated_heal_per_tick,
                        s->pain_per_wrong_answer,
                        s->satiation_per_right_answer);
        return buff;
} else {
        return NULL;
}

Is cleaner this way:

buff = malloc(len + 1);
if (!buff)
        return NULL;
written = snprintf(buff, len + 1, format_string,
                        state->last_update_time,
                        p->health, p->max_health,
                        p->satiation, p->max_satiation,
                        s->milliseconds_per_tick,
                        s->hunger_pain_per_tick,
                        s->hunger_per_tick,
                        s->satiated_heal_per_tick,
                        s->pain_per_wrong_answer,
                        s->satiation_per_right_answer);
if (written < 0)
        goto err;
return buff;

err:
free(buff);
return NULL;

Edited (02/jul/19 after Will comment) to add error checking

Note that snprintf may fail (EILSEQ or EOVERFLOW)

\$\endgroup\$
1
\$\begingroup\$

There's nothing wrong with the other answers, but if you'd prefer a solution more portable than asprinf() and slightly more straightforward than Jerry Coffin's vsnprintf() helper function solution, consider this variable length macro solution:

#define FOO_ASPRINTF(_str, ...) /* replace FOO with your codebase's prefix */ \
do { \
    int byte_c = snprintf(NULL, 0, __VA_ARGS__); \
    (_str) = malloc(byte_c + 1); \
    if (!(_str)) { \
        FOO_MALLOC_ERR; /* should depend on your codebase's malloc() policy */ \
    } \
    sprintf(_str, __VA_ARGS__); \
} while (0)

Which in OP's case might be invoked as:

char * buff = NULL;
CG_ASPRINTF(buff,
    "%lld %f %f %f %f %f %f %f %f %f %f",
    state->last_update_time,
    p->health,
    p->max_health,
    p->satiation,
    p->max_satiation,
    s->milliseconds_per_tick,
    s->hunger_pain_per_tick,
    s->hunger_per_tick,
    s->satiated_heal_per_tick,
    s->pain_per_wrong_answer,
    s->satiation_per_right_answer
);

Some people might be allergic to macros, but in this case it allows easy implementation, avoids any overhead, and is perfectly portable.

\$\endgroup\$
  • \$\begingroup\$ The main problem I see is it doesn't return an error code. It would be more complicated to handle errors. Maybe the GCC extension ({}) would be more appropriate: gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html \$\endgroup\$ – Cacahuete Frito Jul 2 at 4:49
  • \$\begingroup\$ Also, the fact that it modifies a variable without taking its address is something weird (the capital letters mitigate that, but it's still weird). Pedantic: snprintf() may fail: EILSEQ or EOVERFLOW (altough unlikely): pubs.opengroup.org/onlinepubs/9699919799/functions/… \$\endgroup\$ – Cacahuete Frito Jul 2 at 4:52
  • \$\begingroup\$ @CacahueteFrito 1) You can make FOO_MALLOC_ERR expand to whatever you want (e.g., directly returning an int or enum: return FOO_E_MALLOC, provided that your codebase has a consistent policy regarding return values). However, if you don't like that, you could replace the last 4 lines of the macro with if (_str) { sprintf(_str, __VA_ARGS__); } instead, at which point it is exactly the same as the accepted answer above as far as err handling is concerned: check if the resulting buffer is null on the line below the macro invocation vs on the line below the function invocation. \$\endgroup\$ – Will Jul 2 at 6:17
  • 1
    \$\begingroup\$ @CacahueteFrito 2) One could easily modify the macro to add a layer of indirection, such that the 1st parameter becomes &buff instead of buff, but personally I don't really see the need to cross-dress like a function. 3) Indeed, snprinf() may fail, so why doesn't your own answer account for that? \$\endgroup\$ – Will Jul 2 at 6:40
  • \$\begingroup\$ 2) Fine. 3) Touche. I didn't intend to offer a full solution, just to note the reordering; but yes, an answer should not be unsafe even if it isn't trying to be complete. Fixed that. \$\endgroup\$ – Cacahuete Frito Jul 2 at 11:38
1
\$\begingroup\$

Side issue

"%f" does not well express the state of a floating point (FP) object

When a FP is large, printing 100s of digits is not informative. More compact options exist.

Worse, when a FP is much smaller than 1, "%f" only retains a few or zero digits - losing perhaps all precession.

As FP are encoded in exponential notation, use an exponential format

printf("%a", fp);  //Hexadecimal significant with binary exponent
// or 
printf("%.*e", DBL_DECIMAL_DIG - 1, fp);  // Decimal exponential
// or 
printf("%.*g", DBL_DECIMAL_DIG, fp);  // Decimal exponential when needed, else fix point.

ref


Minor

snprintf() returns an int.

Capture result with an int which may be negative due to "negative value if an encoding error occurred."

int ilen = snprintf(....
if (ilen < 0) return NULL; // or some other error indication.

size_t size = ilen + 1u;
char* buff = malloc(size);
...
\$\endgroup\$

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.