2

I have a hardware device connected to the computer and on the hardware device there is 512 bytes of memory for storing parameters. I want to read this data and decode the parameters stored in the memory. I will explain how I currently do this, and then ask if there is a better way.

I read the data using this function call

std::array<uint8_t, 512> device_data = read_parameters_from_device_memory();

The first byte read encodes the version of the data storage format. So I introduce a new type to hold the data and facilitate decoding to different storage formats.

struct device_data_union {
    uint8_t version = 255;
    std::array<uint8_t, 511> data;

    device_data_union() {}
    device_data_union(const std::array<uint8_t, 512>& data_from_device) {
        version = data_from_device[0];
        std::copy(
            data_from_device.begin() + 1, data_from_device.end(), 
            data.begin());
    }

    template <typename T>
    const T* access() const {
        return T::version == version? reinterpret_cast<const T*>(data.data()) : nullptr;
    }

    template <typename T>
    T* access() {
        return T::version == version? reinterpret_cast<T*>(data.data()) : nullptr;
    }
};

The access template method lets me safely decode the data by providing a type T that has a version constant.

I construct a value of this type by calling its constructor on the data that I read:

device_data_union data_union(device_data);

Here are the different data formats:

#pragma pack(push, 1)
struct device_data_v0 {
    static const uint8_t version = 0;

    double a = 3.0;
    double b = 4.0;
    void mapping() {} // 'mapping' is absent for this data format.
};

struct device_data_v1 {
    static const uint8_t version = 1;

    void a() {} // 'a' is absent for this data format.
    double b = 4.0;
    Line mapping;
};

struct device_data_v2 {
    static const uint8_t version = 2;

    double a = 3.0;
    void b() {} // 'b' is absent for this data format.
    Line mapping;
};
#pragma pack(pop)

All these types store a subset of the parameters a, b and mapping. When a parameter is not present for a particular format, I mark it as absent by adding a void method with the name of the parameter. The reason for that will be clear later.

Anyway, I want to unify all these formats into a value of this type that contains all parameters.

struct device_data_unified {
    double a = 3.0;
    double b = 4.0;
    Line mapping;
};

To accomplish that, I wrote a function unify that uses some template specialization to decide at compile time if a field is a variable or a method, and in case it is a variable then it copies its value:

// Unification code
template <typename T, typename P, typename D>
struct CopyParameter {
    static void apply(const T& src, P p, D* dst) {
        *dst = src.*p;
    }
};

template <typename T, typename D>
struct CopyParameter<T, void (T::*)(), D> {
    static void apply(const T&, void (T::*)(), D*) {}
};

template <typename T, typename P, typename D>
void copyParameter(const T& src, P p, D* dst) {
    CopyParameter<T, P, D>::apply(src, p, dst);
}

template <typename T>
void tryCopyParameters(const T* src0, device_data_unified* dst) {
    if (src0 == nullptr) {
        return;
    }
    const auto& src = *src0;

    copyParameter(src, &T::a, &(dst->a));
    copyParameter(src, &T::b, &(dst->b));
    copyParameter(src, &T::mapping, &(dst->mapping));
}

device_data_unified unify(const device_data_union& src) {
    device_data_unified dst;
    tryCopyParameters(src.access<device_data_v0>(), &dst);
    tryCopyParameters(src.access<device_data_v1>(), &dst);
    tryCopyParameters(src.access<device_data_v2>(), &dst);
    return dst;
}

I can now decode the device data like this:

device_data_unified unified = unify(data_union);

Here is my question: While this code works, the template hacks are not very readable. Is there a more readable way of accomplishing this such that (i) I don't have to excessively duplicate code when considering the different formats and (ii) resort to using SFINAE (as it is not entirely supported properly by some common compilers).

The code that I provided above is just an example. In reality, I have more formats and member variables, so even if code duplication would not be a big issue for the particular example above, in reality it would.

Complementary information: This is C++11. I am compiling it using various compilers, such as Clang++ 9, g++ 6.3 and some MSVC compiler from 2017.

  • Your code is actually pretty nice to read, and I like that you brace your single line conditionals. One suggestion style wise: Newline the {} on functions and structs, while keeping them on flow control. Picked this up from kernel.org/doc/html/v4.10/process/… and it helps me differentiate at what I am looking at. For what its worth, I accept templates as being unreadable by default, and its the only place I will permit the use of auto; to offset this, Use excessive commenting. – Akiva Oct 10 at 10:21
  • Which version of C++ are you using? And which compiler? – Jan Dorniak Oct 10 at 19:48
  • @JanDorniak Thanks for reminding me to mention that. I added this information at the bottom of my answer. It is C++11 and it needs to compile also on some older compilers (which is why I do not want to use any SFINAE). – Rulle Oct 15 at 5:49
0

There are surely different ways of solving this without templates, still without duplicating (much) code. If the result is more readable you will have to decide by yourself.

First, you could give device_data_unified three different constructors with parameters of type const device_data_v0&, const device_data_v1& and const device_data_v2&. Those constructors contain a little bit of overlapping functionality (some assignments will occur twice), but that is probably negligible and will be compansated by some boilerplate code saved.

Then, you implement a conversion function along the lines of

device_data_unified unify(const std::array<uint8_t, 512>& data_from_device) 
{
    uint8_t version = data_from_device[0];
    std::array<uint8_t, 511> data;
    std::copy(
        data_from_device.begin() + 1, data_from_device.end(), 
        data.begin());

    switch(version)
    {
        case 0:
            auto ptrData0 = reinterpret_cast<const device_data_v0 *>(&data);
            return device_data_unified(*ptrData0);
        case 1:
            auto ptrData1 = reinterpret_cast<const device_data_v1 *>(&data);
            return device_data_unified(*ptrData1);
        case 2:
            auto ptrData2 = reinterpret_cast<const device_data_v2 *>(&data);
            return device_data_unified(*ptrData2);
    }
}

Note I did not test this with a compiler, maybe there is still some syntactic error, but I think you will get the idea.

And that's it. Decide for yourself if you think the little amount of code duplication here this is better or worse than the necessary boilerplate code which is required for making the templates work.

  • The code duplication is in converting from types representing specific version to the unified format. By writing separate constructors for each version-specific data type you essentially removed all code deduplication which makes this answer a non-solution. Note that OP did not ask to remove templates alltogether but only not to use SFINAE. – Jan Dorniak Oct 10 at 23:04
  • @JanDorniak: as I wrote in my answer, I introduced a little bit of code duplication (as I wrote in my answer), that is right., and this is more than compensated by the saved boilerplate code the template solution requires. Situation may look differently when there are 300 types to unify instead of 3, but in that case, I would not expect 300 constructors to be necessary, there would be other means of avoiding the duplication. – Doc Brown Oct 11 at 1:53
  • I should have clarified that the code I provided in my question is for the sake of brevity an example and, in reality, I have more formats and more member variables to unify. This is why I want to avoid duplication of code (such as writing a device_data_unified constructor for every format I want to unify). – Rulle Oct 15 at 5:53
0

You are trying to cram any version into the same struct. I think I would approach it differently.

You could define a universal interface that has any member ever, from any version. If a member does not apply to a version, the parameter would return as null or as a default value.

Then for each version, create a class that implements that interface. The constructor of each class takes the 512 bytes long array and just stores it without parsing it. A base class could do this.

For each member accessor you would implement code that fetches your parameter from the byte array. Most implementations could be shared, you would implement those in your base class. Maybe for some versions you would have to use a different offset because a parameter was removed or inserted from one version to another. Depending on the differences over versions you would either reimplement an accessor or just inherit the base class's implementation.

Then all you do is determine the version and instantiate the right class, passing in the byte array, and work with the interface.

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.