3
\$\begingroup\$

Ok, a struct would actually be most convenient, but it woudl be easier on teh system to have 2 seperate arrays rather than an array of structs.

#define PAIR(Str) {(Str), sizeof(Str)-1}
static const struct StrStruct {
    const char *Str;
    unsigned int Len;
} Packed4 DefaultStr[] = {
    PAIR("Error"),
    PAIR("An unexpected error occurred"),
    PAIR("while saving data"),
    PAIR("Close some windows or programs and try again"),
    PAIR("Singleplayer"),
    PAIR("Multiplayer"),
    PAIR("Settings"),
    PAIR("Quit"),
    PAIR("Graphics"),
    PAIR("User Interface"),
    PAIR("Sounds"),
    PAIR("Controls"),
    PAIR("Language"),
    PAIR("Compatibility"),
    PAIR("Help"),
    PAIR("About"),
    PAIR("On"),
    PAIR("Off"),
    PAIR("Field of View"),
};

Simply hardcoding them is 'magic numbers'.

static const char *const Strs[] = {
    "Singleplayer",
    "Multiplayer",
    "Settings",
    "Quit",
    "Graphics",
    "User Interface",
    "Sounds",
    "Controls",
    "Language",
    "Compatibility",
    "Help",
    "About",
    "On",
    "Off",
    "Field of View"
};
static const unsigned short Lengths[] = {
    12,
    11,
    8,
    //...
};

So I came up with my current solution, though kind of a pain in the backside, I have this.

#define ERROR "Error"
#define UNEXPECTEDERROR "An unexpected error occurred"
#define WHILESAVINGDATA "while saving data"
#define CLOSEPROGRAMS "Close some windows or programs and try again"
#define SINGLEPLAYER "Singleplayer"
#define MULTIPLAYER "Multiplayer"
#define SETTINGS "Settings"
#define QUIT "Quit"
#define GRAPHICS "Graphics"
#define UI "User Interface"
#define AUDIO "Audio"
#define CONTROLS "Controls"
#define LANGUAGE "Language"
#define COMPATIBILITY "Compatibility"
#define HELP "Help"
#define ABOUT "About"
#define ON "On"
#define OFF "Off"
#define FOV "Field of View"
static const char *const DStr[] = {
    ERROR,
    UNEXPECTEDERROR,
    WHILESAVINGDATA,
    CLOSEPROGRAMS,
    SINGLEPLAYER,
    MULTIPLAYER,
    SETTINGS,
    QUIT,
    GRAPHICS,
    UI,
    AUDIO,
    CONTROLS,
    LANGUAGE,
    COMPATIBILITY,
    HELP,
    ABOUT,
    ON,
    OFF,
    FOV,
};
#define STRLEN(Str) (sizeof(Str)-1)
static const unsigned short DLen[] = {
    STRLEN(ERROR),
    STRLEN(UNEXPECTEDERROR),
    STRLEN(WHILESAVINGDATA),
    STRLEN(CLOSEPROGRAMS),
    STRLEN(SINGLEPLAYER),
    //...
};

Is there a better way to do this? Is there a cleaner way to initialize an constant array of strings, and a constant array of the equivalent lenghts? I'm mainly looking at the last one. How can I improve that one?

\$\endgroup\$
6
  • 1
    \$\begingroup\$ looks like a job for X macros, eg en.wikipedia.org/wiki/X_Macro \$\endgroup\$ – dmuir Sep 25 '20 at 7:24
  • \$\begingroup\$ You can use a struct of arrays. (And a spelling checker.) \$\endgroup\$ – greybeard Sep 25 '20 at 10:38
  • \$\begingroup\$ I don't think sizeof ‹string literal› - 1 is specified to equal strlen(‹string literal›). \$\endgroup\$ – greybeard Sep 25 '20 at 10:43
  • \$\begingroup\$ @greybeard strlen() takes the length of the string, but excludes the nullterm. sizeof on a string literal takes the total size, including the null terminator. So strlen() on a string literal is equivalent to sizeof - 1. \$\endgroup\$ – user231012 Sep 25 '20 at 14:20
  • \$\begingroup\$ @user231012 strlen(string_literal) != (sizeof string_litleral - 1) when then string literal has an explicit \0. e.g. strlen("abc\0xyz") --> 3, else you are mostly OK. \$\endgroup\$ – chux - Reinstate Monica Sep 25 '20 at 17:32
4
\$\begingroup\$

To expand on my comment, the X macro technique can be used for this. See here, for example

The idea is that we define the list once, for example

#define STRINGLIST \
X( "alice") \
X( "bob") \
X( "cat")

When we want to use this list, we invoke the above macro, having defined the macro X:

static const char *const DStr[] = {
#define X(S) S,
STRINGLIST
#undef X
};

static const unsigned short DLen[] = {
#define X(S) sizeof( S)-1,
STRINGLIST
#undef X
};

This way we only have to define the strings once, and guarantee that the arrays DStr and DLen are in the same order. The disadvantage is that it looks pretty bizarre first time you see it, and others maintaining your code might be boggled.

\$\endgroup\$
2
  • \$\begingroup\$ The disadvantage is that it looks pretty bizarre first time you see it, and others maintaining your code might be boggled. Well, I'm the only one maintaining my code so I guess this really works then :) \$\endgroup\$ – user231012 Sep 25 '20 at 14:44
  • \$\begingroup\$ Also, the strlen() function call return values aren't compile time constant, so it should be #define X(S) (sizeof(S)-1 \$\endgroup\$ – user231012 Sep 25 '20 at 14:48
1
\$\begingroup\$

I'm mainly looking at the last one. How can I improve that one?

A common problem is the number of elements to Strs[], Lengths[] will differ due to a maintenance error.

After the definitions, add a _Static_assert or the like to detect that problem.

_Static_assert(sizeof Strs/sizeof Strs[0] == sizeof Lengths/sizeof Lengths[0], 
    "Strs Lengths size mismatch");

Unclear why code uses unsigned short vs. unsigned char (for space efficiency) nor size_t (for generality). In any case, I'd expect the compiler to whine if the type was too narrow.

static const unsigned short DLen[]
//           ^------------^ ???

If code uses the last approach, consider _ for spaces; easier to read.

// #define WHILESAVINGDATA "while saving data"
#define WHILE_SAVING_DATA "while saving data"
\$\endgroup\$
0
\$\begingroup\$

You already have a working solution but you say "it would be easier on the system to have 2 separate arrays". I have no idea what you mean by that, and in any case this is false since memory locality plays a big role in performance and by separating these lengths from the string contents you'd be prematurely pessimizing your code for no reason.

But of course you haven't mentioned the biggest obstacle: how are you going to refer to those strings in that array? Surely not by magic numbers. Thus: by some separate enums? No, forget about using preprocessor macros: yuck! The less of those, the better. How will you ensure the names for the indices remain in sync with contents of the array?

That's the problem with C: it's a really nice language for outputting generated code to, but by itself it's like writing in a human-friendly assembly almost, and almost any idea you have can't really be expressed nicely in C without writing absurd amounts of code.

I don't know why you insist on having an array, since just naming each string would give you names that are intimately tied to the contents, but there are valid reasons to have an array, e.g. for translations - so let's say the array is a valid requirement.

So, what you want really is code generation. None of this stuff should be manually tweaked by humans - it's a total waste of time, and obscure macros doesn't help with readability either. Let the generator generate plain C, with nothing special.

You decided to use C, so I can propose a code generator written in C, even though C is rather hard to get right and becomes somewhat verbose. Again: it is something I came up with in 10 minutes, it's just an example that should be made much nicer (and even longer) if you intend to use it:

#include <assert.h>
#include <ctype.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>


void *check_alloc(void *ptr, size_t size)
{
    if (size && !ptr)
    {
        fprintf(stderr, "Out of memory while attempting to allocate %zu bytes\n", size);
        exit(1);
    }
    return ptr;
}

void *checked_malloc(size_t size)
{
    return check_alloc(malloc(size), size);
}

void *checked_realloc(void *ptr, size_t size)
{
    return check_alloc(realloc(ptr, size), size);
}

typedef struct {
    char *data;
    size_t count, size;
} Buffer;

Buffer buf_new(void)
{
    Buffer buf = {.data = NULL, .count = 0, .size = 4096 };
    buf.data = checked_malloc(buf.size);
    return buf;
}

char *buf_end(const Buffer *b)
{
    return b->data + b->count;
}

size_t buf_avail(const Buffer *b)
{
    return b ? (b->size - b->count) : 0;
}

size_t buf_extend(Buffer *b)
{
    size_t const newSize = 2*b->size;
    char *newData = checked_realloc(b, newSize);
    b->size = newSize;
    b->data = newData;
    return b->size - b->count;
}

void buf_append(Buffer *b, size_t count)
{
    b->count += count;
}

void buf_free(Buffer *b)
{
    if (b) {
        free(b->data);
        memset(b, 0, sizeof(*b));
    }
}

Buffer read_all(FILE *file)
{
    Buffer buf = buf_new();
    if (!buf.size) return buf;
    for (;;)
    {
        size_t maxToRead = buf_avail(&buf);
        if (!maxToRead)
            maxToRead = buf_extend(&buf);
        if (!maxToRead)
            break;
        size_t readNow = fread(buf_end(&buf), 1, maxToRead, file);
        buf_append(&buf, readNow);
        if (!readNow) {
            if (feof(stdin))
            {
                *buf_end(&buf) = '\0'; 
                return buf;
            }
            if (ferror(stdin))
                break;
        }
    }
    buf_free(&buf);
    return buf;
}

int isident1(int c) { return isalpha(c) || c == '_' || c == '$'; }
int isident(int c) { return isalnum(c) || c == '_' || c == '$'; }
int isendl(int c) { return c == '\r' || c == '\n'; }

typedef struct {
    char *data;
    size_t size;
} StringView;

static const StringView empty_str[1];

char str_last(const StringView *str)
{
    return str->size ? str->data[str->size-1] : '\0';
}

char *str_lastp(const StringView *str)
{
    return str->size ? str->data + str->size - 1 : NULL;
}

void buf_append_stringz(Buffer *buf, const StringView *str)
{
    for (size_t avail = buf_avail(buf); avail < str->size + 1;)
    {
        avail = buf_extend(buf);
    }

    memcpy(buf_end(buf), str->data, str->size);
    buf_append(buf, str->size);
    *buf_end(buf) = '\0';
    buf_append(buf, 1);
}

StringView read_label(char **input)
{
    StringView result = {.data = NULL, .size = 0};
    char *p = *input;
    unsigned char c;
    while ((c = *p) && isspace(c)) ++p;
    if (!c) return result;
    result.data = p;
    if ((c = *p) && isident1(c)) ++p;
    else return result;
    while ((c = *p) && isident(c)) ++p;
    result.size = p - result.data;
    if (c) *p++ = '\0'; // null-terminate the result
    *input = p;
    return result;
}

StringView read_text_line(char **input)
{
    StringView result = {.data = NULL, .size = 0};
    char *p = *input;
    unsigned char c;
    while ((c = *p) && isspace(c)) ++p;
    if (!c) return result;
    result.data = p;
    while ((c = *p) && !isendl(c)) ++p;
    result.size = p - result.data;
    if (c) *p++ = '\0'; // null-terminate the result
    *input = p;
    return result;
}

int main(int argc, char **argv)
{
    // Arguments
    // <array_name>
    
    // Input format:
    // <label> <whitespace> <text to go with the label> <newline>
    // The text can contain C escapes, which are not interpreted.
    // Multi-line strings are supported using the line continuation character
    // <\> at the end of the line.
    
    if (argc != 2) return 1;
    const char *array_name = argv[1];    
    
    Buffer labels = buf_new();
    Buffer input = read_all(stdin);
    
    fprintf(stdout, "const StringView %s[] = {\n", array_name);

    int has_previous_entry = 0;
    for(char *in = input.data;;)
    {
        StringView label = read_label(&in);
        if (!label.size) break;

        int needs_open_brace = 1;
        size_t total_size = 0;
        for (;;) 
        {
            StringView text = read_text_line(&in);
            if (!text.size) break;
            total_size += text.size;
            if (has_previous_entry && needs_open_brace)
                fprintf(stdout, ",\n");
            if (needs_open_brace)
            {
                buf_append_stringz(&labels, &label);
                fprintf(stdout, "    /* %s */\n    { ", label.data);
            }
            else
                fprintf(stdout,               "      ");
            needs_open_brace = 0;
            has_previous_entry = 1;

            if (str_last(&text) == '\\')
            {
                *str_lastp(&text) = '\0';
                text.size--;
                fprintf(stdout, "\"%s\"\n", text.data);
                continue;
            }
            fprintf(stdout, "\"%s\", %zu }", text.data, total_size);
            break;
        }
    }
    if (has_previous_entry)
        fprintf(stdout, "\n};\n");
    else
        fprintf(stdout, "};\n");
    
    if (has_previous_entry)
    {
        buf_append_stringz(&labels, empty_str);
        int has_previous_label = 0;
        char *label = labels.data;
        assert(*label);

        fprintf(stdout, "enum %s_labels {\n", array_name);
        while (*label)
        {
            size_t len = strlen(label);
            if (has_previous_label)
                fprintf(stdout, ",\n");
            fprintf(stdout, "    %s", label);
            label += len + 1;
            has_previous_label = 1;
        }
        fprintf(stdout, "\n};\n");
    }

    return 0;
}

Invoked as generate myArray, given the following standard input:

label_1 text1a text1b
label_2 text2a text2b text2c\
  text2d text2e \
  text2f

the output is:

const StringView myArray[] = {
    /* label_1 */
    { "text1a text1b", 13 },
    /* label_2 */
    { "text2a text2b text2c"
      "text2d text2e "
      "text2f", 42 }
};
enum myArray_labels {
    label_1,
    label_2
};

For type-safety, you'd also want the generator to emit a custom array lookup function so that the wrong enum type would at least be warned against by the compiler (C is insane in that everything that's not a pointer or a struct behaves as if it was an integer).

StringView *myArray_get(enum myArray_labels label)
{
  assert(label < 2);
  return myArray[label];
}

If you are OK with not using C for the generator, then either C++ or Python or Perl or even bash would yield a more robust generator at less than 1/3 the size.

Let's say we wanted to integrate the above code generator - let's call it strarraygen - into cmake. It'd look as follows:

# This is whatever target you use the generated file in
add_executable(your_primary_target
    …
    "${CMAKE_CURRENT_BINARY_DIR}/myArray.c")

# This is the code generator target
add_executable(strarraygen strarraygen.c)

# This generates the array based on description in `myArray.txt`
add_custom_command(OUTPUT myArray.c
    COMMAND "$<TARGET_FILE:strarraygen>" myArray
            < "${CMAKE_CURRENT_SOURCE_DIR}/myArray.txt" 
            > myArray.c
    DEPENDS myArray.txt strarraygen )

If you use cmake, then you probably shouldn't be writing such a generator in C since it'll end up being 10x (or worse!) longer than the equivalent CMake script.

\$\endgroup\$
5
  • \$\begingroup\$ memset(b, 0, sizeof(b)); in buf_free() is unclear. Why use the size of a pointer to determine how much to clear? \$\endgroup\$ – chux - Reinstate Monica Sep 26 '20 at 2:57
  • \$\begingroup\$ Personally I am much in favour of code generation, and use it when I can. However the problem I always have is how to integrate the code generation into the build system. In the present case it wouldn't be too bad, i suppose, to put Dstr and Dlen etc into a file that contains just that, but still artificial. And the more generated bits you have, the more you end up with a wheen of wee files which is bad news for readability. \$\endgroup\$ – dmuir Sep 28 '20 at 15:09
  • \$\begingroup\$ If the build system isn't braindead, integration is less text than your comment :) As much as cmake if loaded with obscure historical baggage, they have got this part well under control :) It really is less text! \$\endgroup\$ – Kuba hasn't forgotten Monica Sep 30 '20 at 23:31
  • \$\begingroup\$ "this is false since memory locality plays a big role in performance" It is not at all obvious that storing strings together with data lead to better performance. In this case the OP isn't actually storing copies of string literals, but pointers to string literals. How string literals are stored and organized is handled by the compiler, but they are not going to end up in the same memory segment as integer sizes no matter what you do, so there are no data cache benefits from using structs here. Merging them together is the pre-mature optimization. \$\endgroup\$ – Lundin Oct 1 '20 at 9:10
  • \$\begingroup\$ "No, forget about using preprocessor macros: yuck!" The example posted by the OP is indeed yuck. X macros are however a quite sensible solution in this case, it is an industry standard way of writing code. Unlike coming up with some home-made macro solution, which is indeed bad. It's a much better solution that code generation, since code generation increases complexity and the chances for bugs. Especially the horribly obfuscated code you posted here; which could do with a serious code review of its own. \$\endgroup\$ – Lundin Oct 1 '20 at 9:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy