Generic type data storage

Question

We have a module which can be used by user to store and load value of variables. Every variable has an index associated with it (in the shown code cample this is ommitted and index is instead the data type) which refers to index in data table.

Shown code below is a minimum working example of what we currently have. To store variable value user inputs data type (or variable index) and pointer to variable which he wants to store as arguemnts to write() function. When user wants to load value of variable he will use read() function.

/* This is inside of module */
#include <stdint.h>

enum t {t_u8, t_s8, t_u16, t_s16, t_u32, t_s32, t_f};
static uint32_t data[7];

void write(enum t t, void *val)
{
    switch (t) {
    case t_u32:
    case t_s32:
    case t_f: data[t] = *((uint32_t *)val); break;
    case t_u16:
    case t_s16: data[t] = *((uint16_t *)val); break;
    case t_u8:
    case t_s8: data[t] = *((uint8_t *)val); break;
    }
}

void read(enum t t, void *val)
{
    switch (t) {
    case t_u32:
    case t_s32:
    case t_f: *((uint32_t *)val) = *((uint32_t *)&data[t]); break;
    case t_u16:
    case t_s16: *((uint16_t *)val) = *((uint16_t *)&data[t]); break;
    case t_u8:
    case t_s8: *((uint8_t *)val) = *((uint8_t *)&data[t]); break;
    }
}

/* User of module */
int main(void)
{
    uint8_t u8 = 123;
    int8_t s8 = -55;
    uint16_t u16 = 4992;
    int16_t s16 = -31829;
    uint32_t u32 = 3009238113;
    int32_t s32 = -123394810;
    float f = -1234.99f;

    write(t_u8, &u8); read(t_u8, &u8);
    write(t_s8, &s8); read(t_s8, &s8);
    write(t_u16, &u16); read(t_u16, &u16);
    write(t_s16, &s16); read(t_s16, &s16);
    write(t_u32, &u32); read(t_u32, &u32);
    write(t_s32, &s32); read(t_s32, &s32);
    write(t_f, &f); read(t_f, &f);

    return 0;
}

We want this code to be as portable and simple and easy to understand as possible without adding a lot of complexity.

You can assume that val will always point to variable of correct type. What are the dangers of this code? One issue I see is that strict aliasing rule is violated multiple times. Are there any other pitfalls present? How would you change write() and read() functions to improve them?

Answer 1

Answers to your questions

What are the dangers of this code?

• Use of globals, as mentioned in coderodde's answer. What if you want two generic variables?
• There is no way to tell which element(s) of data[] hold a valid value. The common solution is to use a tagged union, as shown in the second part of Bas Visscher's answer.

One issue I see is that strict aliasing rule is violated multiple times. Are there any other pitfalls present?

You have in fact not violated the strict aliasing rule. Each element of data[] is only accessed through one type of pointer, so there is never access through a pointer of a different type.

You also do not have any pointer alignment issues.

However, your intuition tells you this pointer casting is dangerous, and rightfully so; it is easy to make a mistake when doing this. It is better to avoid it, for example by using the aforementioned tagged union.

How would you change write() and read() functions to improve them?

First, avoid globals by passing a pointer to the generic variable you want to access to your read() and write() functions.

Then, it depends on what you want to use this for. If you know at compile time which member you want to access, like in write(t_u8, &u8), then I would create functions where the type is part of the name, like write_u8(&u8), or rather just define a union and use it directly:

union t {
    uint8_t u8;
    uint16_t u16;
    …
};

union t data;
data.u8 = 42; // replaces write(t_u8, 42);

If you don't know the type at compile time, then I would go for the tagged union approach from Bart Visscher's answer.

Consider using C++

If you are not forced to use C, consider using C++ instead. The advantage of C++ is that it allows you to write type-safe code. For example, you could use std::variant and rewrite your main() to:

using t = std::variant<std::uint8_t, …, float>;

int main()
{
    uint8_t u8 = 123;
    …
    float f = -1234.99f;

    t data;

    data = u8; u8 = std::get<std::uint8_t>(data);    
    …
    data = f; f = std::get<float>(data);
}

And it would automatically throw an exception at runtime if you did something like:

data = u8; std::get<float>(data);

Answer 2

Advice I - Don't use globals

static uint32_t data[7];

Using global data may introduce rather harsh (potential) debugging difficulties. I would wrap the state in a nice struct.

Advice II - Don't waste the space

If you have rolled your own struct for holding generic data, you would use 1 + 1 + 2 + 2 + 4 + 4 + 4 = 18 bytes. Your version uses 7 * 4 = 28.

Possible alternative implementation

I had the following in mind:

data_storage.h:

#ifndef DATA_STORAGE_H
#define DATA_STORAGE_H

#include <stdint.h>

typedef struct data_storage_t {
    int8_t signed_integer_8;
    uint8_t unsigned_integer_8;
    float floating_point_32;
} data_storage_t;

void data_storage_init(data_storage_t* ds);

void data_storage_write_signed_integer_8   (data_storage_t* ds, int8_t  signed_integer_8);
void data_storage_write_unsigned_integer_8 (data_storage_t* ds, uint8_t unsigned_integer_8);
void data_storage_write_floating_point     (data_storage_t* ds, float   floating_point_32);

int8_t  data_storage_read_signed_integer_8   (data_storage_t* ds);
uint8_t data_storage_read_unsigned_integer_8 (data_storage_t* ds);
float   data_storage_read_floating_point_32  (data_storage_t* ds);

#endif /* DATA_STORAGE */

data_storage.c:

#include "data_storage.h"

void data_storage_init(data_storage_t* ds) {
    ds->signed_integer_8   = 0;
    ds->unsigned_integer_8 = 0;
    ds->floating_point_32  = 0.0f;
}

void data_storage_write_signed_integer_8(data_storage_t* ds,
                                         int8_t signed_integer_8) {
    ds->signed_integer_8 = signed_integer_8;
}

void data_storage_write_unsigned_integer_8(data_storage_t* ds, 
                                           uint8_t unsigned_integer_8) {
    ds->unsigned_integer_8 = unsigned_integer_8;
}

void data_storage_write_floating_point(data_storage_t* ds, 
                                       float floating_point_32) {
    ds->floating_point_32 = floating_point_32;
}

int8_t data_storage_read_signed_integer_8(data_storage_t* ds) {
    return ds->signed_integer_8;
}

uint8_t data_storage_read_unsigned_integer_8(data_storage_t* ds) {
    return ds->unsigned_integer_8;
}

float data_storage_read_floating_point_32(data_storage_t* ds) {
    return ds->floating_point_32;
}

main.c:

#include "data_storage.h"
#include <stdio.h>

int main() {
    data_storage_t ds;
    data_storage_init(&ds);

    data_storage_write_signed_integer_8(&ds, -1);
    data_storage_write_unsigned_integer_8(&ds, 2);
    data_storage_write_floating_point(&ds, 3.5f);

    printf(
        "%d\n%d\n%f\n",
        data_storage_read_signed_integer_8(&ds),
        data_storage_read_unsigned_integer_8(&ds),
        data_storage_read_floating_point_32(&ds));

    return 0;
}

Answer 3

Sorry, late to the party.

Are there any other pitfalls present?

Missed opportunity for _Generic

This is the biggest weakness.

"Function overloading is not possible in C" is not completely true.

Code could use _Generic (since C11) to steer type tagging and function selection.

This involves a fairly large code edit.

A read() (which is a macro) could steer to read_u32(), read_i16(), ....

Then code will be simpler and isn't this what you really want?

// write(t_u8, &u8); read(t_u8, &u8);
write(&u8); read(&u8);

Code assumes float is 32-bit

Somewhat safe, yet still an assumption.

Better to add an assertion:

_Static_assert(sizeof(float) == sizeof(uint32_t), "Unexpected float size");

Even better drop the assumption,

// static uint32_t data[7];
union {
  uint32_t u32,
  int32_t s32,
  uint16_t u16,
  int16_t s16,
  uint8_t u8,
  int8_t s8,
  float f;
} data[7];

Missing char

char is far more common that int8_t and uint8_t and deserves inclusion.

Collison risk high

read() and write() are far too common. Use a different naming scheme.

Avoid magic numbers

Get rid of 7.

// enum t {t_u8, t_s8, t_u16, t_s16, t_u32, t_s32, t_f};
// static uint32_t data[7];
enum t {t_u8, t_s8, t_u16, t_s16, t_u32, t_s32, t_f, t_N};
static uint32_t data[t_N];

Needlessly does not allow a const pointer

write should allow pointers to a const

// void write(enum t t, void *val)
void write(enum t t, const void *val)

---

Good use of f to avoid double rounding

float f = -1234.99f;

Answer 4

How about something like this? Now, your code won't compile if you want to write a type that isn't supported. By overloading the same function with different types, the user isn't annoyed by using the right type.

EDIT: Function overloading is not possible in C. –G. Sliepen In this modified code, each function name includes a suffix that specifies the data type it operates on (uint8, uint16, or uint32). This makes the function names unique and self-explanatory. The compiler simply throws an error if the types don't match.

But I guess you don't want to do that, a bit more context to your question would help us help you.

typedef union
{
    uint8_t uint8;
    uint16_t uint16;
    uint32_t uint32;

} data_t;

static data_t buffer;


void write_uint8(uint8_t val)
{
    buffer.uint8 = val;
}

void write_uint16(uint16_t val)
{
    buffer.uint16 = val;
}

void write_uint32(uint32_t val)
{
    buffer.uint32 = val;
}

void read_uint8(uint8_t* val)
{
    *val = buffer.uint8;
}

void read_uint16(uint16_t* val)
{
    *val = buffer.uint16;
}

void read_uint32(uint32_t* val)
{
    *val = buffer.uint32;
}

EDIT2: If you really want to stick to the void pointer solution, you could do something like this:

#include <stdint.h>

enum dataType_t { t_u8, t_u16 };

typedef union
{
    uint8_t uint8;
    uint16_t uint16;
} data_t;

typedef struct
{
    enum dataType_t type;
    data_t data;
} stored_t;

static stored_t buffer;

void write(enum dataType_t type, void* val)
{
    buffer.type = type;
    switch (type) {  // Corrected variable name
    case t_u16:
        buffer.data.uint16 = *((uint16_t*)val);
        break;
    case t_u8:
        buffer.data.uint8 = *((uint8_t*)val);
        break;
    default:
        // Handle the error case here, e.g., print an error message or return an error code
        break;
    }
}

void read(enum dataType_t type, void* val)
{
    if (type != buffer.type)
    {
        // Handle the type mismatch error here
        return;
    }

    switch (type) {  // Corrected variable name
    case t_u16:
        *((uint16_t*)val) = buffer.data.uint16;
        break;
    case t_u8:
        *((uint8_t*)val) = buffer.data.uint8;
        break;
    default:
        // Handle the error case here, e.g., print an error message or return an error code
        break;
    }
}